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.

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

Already on GitHub? Sign in to your account

Use a more specific condition for debug output. #171

Merged
merged 3 commits into from May 16, 2017
Merged

Use a more specific condition for debug output. #171

merged 3 commits into from May 16, 2017

Conversation

pascaliske
Copy link
Contributor

@pascaliske pascaliske commented May 12, 2017

When someone uses visionmedia/debug for logging he has to set process.env.DEBUG for enabling his own log messages.
He always sees the following output, because node-notifier only checks for the presence of the env var (if (process.env.DEBUG)):

node-notifier debug info (fileCommandJson):
[notifier path] [...]/node_modules/node-notifier/vendor/terminal-notifier.app/Contents/MacOS/terminal-notifier
[notifier options] -title "xyz" -message "馃帀 some message here" -json "true"

Instead of checking for the presence you could check for the package name inside of the env var. Then someone could enable debug logging for node-notifier with setting the env var to something like this:

process.env.DEBUG = 'some-other,notifier';

@mikaelbr
Copy link
Owner

Hi. Thanks for this. Didn't think would be an issue. Do you know, is it common to do DEBUG with operations like this, or would a better way to do e.g. process.env.NODE_NOTIFIER_DEBUG = true?

@pascaliske
Copy link
Contributor Author

pascaliske commented May 16, 2017

Hi, Yes I would say that this is a common usage. See the Usage-section of visionmedia/debug. 馃槂

@mikaelbr
Copy link
Owner

I'm merging this, but will have to do a major release as it is a major change. Will wait for the change in terminal-notifier as that is also a breaking change.

@mikaelbr mikaelbr merged commit 95e54ac into mikaelbr:master May 16, 2017
@mikaelbr
Copy link
Owner

Thanks for your work 馃帀

@pascaliske
Copy link
Contributor Author

Yeah, sure! Will use the master branch until you released the major version!

Thanks for merging, keep up the great work! 馃馃徎

pascaliske pushed a commit to pascaliske/modern-cli that referenced this pull request Aug 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants