FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

util: make util.debuglog() consistent with doc by vsemozhetbyt · Pull Request #13841 · nodejs/node · GitHub

/ node Public

util: make util.debuglog() consistent with doc - #13841

Closed
vsemozhetbyt wants to merge 1 commit into
nodejs:masterfrom
vsemozhetbyt:util.debuglog
ConversationCommits (1)ChecksFiles changed
Closed

util: make util.debuglog() consistent with doc#13841
vsemozhetbyt wants to merge 1 commit into
nodejs:masterfrom
vsemozhetbyt:util.debuglog

Conversation

vsemozhetbyt commented Jun 21, 2017

Copy link
Copy Markdown
Contributor
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

util

Fixes: #13728

nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Jun 21, 2017

vsemozhetbyt commented Jun 21, 2017

Copy link
Copy Markdown
Contributor Author

I am not sure if this is semver-major or semver-patch: this is more consistent with the doc but this may change some logging behavior, fixing any known errors in userland and surprising workarounds.

vsemozhetbyt commented Jun 21, 2017

Copy link
Copy Markdown
Contributor Author

CI: https://ci.nodejs.org/job/node-test-pull-request/8772/

cjihrig reviewed Jun 21, 2017
View reviewed changes
Comment thread lib/util.js Outdated

cjihrig Jun 21, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Would it make more sense to do this on line 151?

vsemozhetbyt Jun 21, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is it OK it would be worthless to do all this if debugs[set] is already true?

vsemozhetbyt Jun 21, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Maybe we should place all the process.env.NODE_DEBUG processing in the if clause?

cjihrig Jun 21, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The code on line 151, inside the debugEnviron === undefined check, should only run once though, right? In the current position, it will run each time a new set is introduced.

vsemozhetbyt Jun 21, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Oh, I did not notice it is used only in this function. So we can make it the array and use it instead of sets, right?

cjihrig Jun 21, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

If you mean making debugEnviron a Set, then yes, that works for me.

Comment thread test/sequential/test-util-debug.js Outdated

cjihrig Jun 21, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why the uppercase variable name?

vsemozhetbyt Jun 21, 2017 •
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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

To differ from section and to clarify the difference. What would you propose? Just reassign section? Or name it like sectionUpperCase?

cjihrig Jun 21, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

You could probably even get away with just using section.toUpperCase() and child.pid. They're only used to create an error message in a test.

vsemozhetbyt commented Jun 21, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@cjihrig Comments addressed. I have shortened the output data a bit to make test string more wrappable.

New CI: https://ci.nodejs.org/job/node-test-pull-request/8774/

vsemozhetbyt commented Jun 21, 2017

Copy link
Copy Markdown
Contributor Author

2 unstable results due to flaky async-hooks/test-callback-error.

vsemozhetbyt commented Jun 21, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

So will we backport it or consider it semver-major?
All added test cases fail with the previous realization.

cjihrig commented Jun 21, 2017

Copy link
Copy Markdown
Contributor

I have no idea how many people may be relying on the existing behavior. It also doesn't seem to be very high priority. I'd say semver major just to be safe, but I'm fine with either since it technically brings behavior closer to the documentation.

vsemozhetbyt added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 21, 2017

vsemozhetbyt commented Jun 21, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I've set semver major for now, please, remove and backport if it will be reconsidered.

jasnell approved these changes Jun 22, 2017
View reviewed changes

jasnell left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Code changes look fine but would you mind adding some detail to the commit message about how the code is closer aligned to the docs

util: make util.debuglog() consistent with doc
18214ee 
Previous realization produces some false positive and false negative
results due to:

* conflicts between unescaped user input and RegExp special characters;

* conflicts between parsing with `\b` RegExp symbol and non
  alphanumeric characters in section names.

Fixes: #13728

vsemozhetbyt commented Jun 22, 2017

Copy link
Copy Markdown
Contributor Author

@jasnell Hopefully done.

vsemozhetbyt added a commit that referenced this pull request Jun 23, 2017
util: make util.debuglog() consistent with doc
3b0e800 
Previous realization produces some false positive and false negative
results due to:

* conflicts between unescaped user input and RegExp special characters;

* conflicts between parsing with `\b` RegExp symbol and non
  alphanumeric characters in section names.

The doc does not mention any such restrictions.

PR-URL: #13841
Fixes: #13728
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

vsemozhetbyt commented Jun 23, 2017

Copy link
Copy Markdown
Contributor Author

Landed in 3b0e800

vsemozhetbyt deleted the util.debuglog branch June 23, 2017 16:50
jasnell mentioned this pull request Oct 30, 2017
Merged
BridgeAR mentioned this pull request Dec 13, 2017
Closed
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
cjihrig cjihrig approved these changes

Assignees

No one assigned

Labels

semver-major PRs that contain breaking changes and should be released in the next major version. util Issues and PRs related to the built-in util module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL