Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from
Oct 8, 2018
Merged

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

merged 1 commit into from
Oct 8, 2018

Conversation

mblarsen
Copy link
Contributor

@mblarsen mblarsen commented Oct 4, 2018

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
Copy link

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
Copy link

Coverage Status

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

@coveralls
Copy link

coveralls commented Oct 4, 2018 •
edited
Loading

Coverage Status

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

@mblarsen
Copy link
Contributor Author

mblarsen commented Oct 4, 2018

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

@Qix-
Copy link
Member

Qix- commented Oct 5, 2018 •
edited
Loading

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

@mblarsen
Copy link
Contributor Author

mblarsen commented Oct 5, 2018

They were added already ac5a726#diff-1dd241c4cd3fd1dd89c570cee98b79ddR85

But spotting a spelling error. Will just correct it.

@mblarsen
Copy link
Contributor Author

mblarsen commented Oct 5, 2018

Fixed

@Qix-
Copy link
Member

Qix- commented Oct 5, 2018

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

@mblarsen
Copy link
Contributor Author

mblarsen commented Oct 5, 2018 •
edited
Loading

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
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));
Copy link
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?

Copy link
Contributor Author

@mblarsen mblarsen Oct 5, 2018 •
edited
Loading

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.

Copy link
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-
Copy link
Member

Qix- commented Oct 5, 2018

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
Copy link
Contributor Author

mblarsen commented Oct 5, 2018

There you go!

mblarsen
mblarsen commented Oct 8, 2018
View reviewed changes
README.md Show resolved Hide resolved
@Qix- Qix- merged commit 7ef8b41 into debug-js:master Oct 8, 2018
@Qix-
Copy link
Member

Qix- commented Oct 8, 2018

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-