Skip to content

feat: Return namespaces string when invoking disable() - #624

Merged
Qix- merged 1 commit into
debug-js:masterfrom
mblarsen:feat-disable-return-value
Oct 8, 2018
Merged

feat: Return namespaces string when invoking disable()#624
Qix- merged 1 commit into
debug-js:masterfrom
mblarsen:feat-disable-return-value

Conversation

@mblarsen

@mblarsen mblarsen commented Oct 4, 2018

Copy link
Copy Markdown
Contributor

PR for feature request #523

I thought about adding a new property createDebug which would hold the untouched string passed to enable but decided against it and instead let the disable deal with what it has: names, skips.

Let me know if you prefer adding an untouched namespaces string to createDebug instead or you any other changes.

Closes #523

ps: Happy Hacktoberfest

@coveralls

coveralls commented Oct 4, 2018

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.5%) to 82.609% when pulling b828357 on mblarsen:feat-disable-return-value into 4236585 on visionmedia:master.

1 similar comment
@coveralls

coveralls commented Oct 4, 2018

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.5%) to 82.609% when pulling b828357 on mblarsen:feat-disable-return-value into 4236585 on visionmedia:master.

@coveralls

coveralls commented Oct 4, 2018 •
edited
Loading

Copy link
Copy Markdown

Coverage Status

Coverage increased (+4.5%) to 87.597% when pulling 692b107 on mblarsen:feat-disable-return-value into 4236585 on visionmedia:master.

@mblarsen

mblarsen commented Oct 4, 2018

Copy link
Copy Markdown
Contributor Author

Let me add test cases if you want and if you are okay with the implementation so we can get the coverage up again :)

@mblarsen
mblarsen force-pushed the feat-disable-return-value branch from b828357 to ac5a726 Compare October 4, 2018 02:06
@Qix-

Qix- commented Oct 5, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Sure, this is fine. :) Yes to test cases, please.

@mblarsen

mblarsen commented Oct 5, 2018

Copy link
Copy Markdown
Contributor Author

They were added already ac5a726#diff-1dd241c4cd3fd1dd89c570cee98b79ddR85

But spotting a spelling error. Will just correct it.

@mblarsen
mblarsen force-pushed the feat-disable-return-value branch from ac5a726 to 1844758 Compare October 5, 2018 02:21
@mblarsen

mblarsen commented Oct 5, 2018

Copy link
Copy Markdown
Contributor Author

Fixed

@Qix-

Qix- commented Oct 5, 2018

Copy link
Copy Markdown
Member

It might seem redundant but can you add a few tests that test passing the returned string back to .enable()?

@mblarsen
mblarsen force-pushed the feat-disable-return-value branch from 1844758 to 99765cc Compare October 5, 2018 06:42
@mblarsen

mblarsen commented Oct 5, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I added a test of using enable. I'm using the names and skips to check for equality.

The initial disable test was extended to also check those to ensure that they were in fact empty before enabling again. Maybe not necessary, but now it is there :)

Qix-
Qix- reviewed Oct 5, 2018
View reviewed changes
Comment thread test.js
expect(namespaces).to.equal('test,abc*,-abc');
debug.enable(namespaces);
expect(oldNames.map(String)).to.deep.equal(debug.names.map(String));
expect(oldSkips.map(String)).to.deep.equal(debug.skips.map(String));

@Qix- Qix- Oct 5, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the .map(String) here?

@mblarsen mblarsen Oct 5, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are RegExp objects, so they are not the same even though the regular expression is the same. Mapping to their string representation will make the chai deep equal happy.

@Qix- Qix- Oct 5, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, sounds good.

@Qix- Qix- self-assigned this Oct 5, 2018
@Qix- Qix- added feature This proposes or provides a feature or enhancement change-minor This proposes or provides a change that requires a minor release labels Oct 5, 2018
@Qix-

Qix- commented Oct 5, 2018

Copy link
Copy Markdown
Member

Last thing: could you add a bit of documentation to the readme?

https://github.com/visionmedia/debug#set-dynamically

@Qix- Qix- added the needs-documentation This issue or change requires additional documentation label Oct 5, 2018
@mblarsen
feat: Return namespaces string when invoking disable()
692b107 
feat: Add unit tests for disable return value

fix: Correct spelling in test case description

feat: Test that disable-string works with enable again

Closes debug-js#523

docs: Add section about disable return value
@mblarsen
mblarsen force-pushed the feat-disable-return-value branch from 99765cc to 692b107 Compare October 5, 2018 13:37
@mblarsen

mblarsen commented Oct 5, 2018

Copy link
Copy Markdown
Contributor Author

There you go!

mblarsen
mblarsen commented Oct 8, 2018
View reviewed changes
Comment thread README.md Resolved
@Qix-
Qix- merged commit 7ef8b41 into debug-js:master Oct 8, 2018
@Qix-

Qix- commented Oct 8, 2018

Copy link
Copy Markdown
Member

Thank you again :) I'll get this released shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Qix- Qix- Qix- left review comments

Assignees

@Qix- Qix-

Labels

change-minor This proposes or provides a change that requires a minor release feature This proposes or provides a feature or enhancement needs-documentation This issue or change requires additional documentation

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mblarsen @coveralls @Qix-