Uh oh!
There was an error while loading. Please reload this page.
util: make util.debuglog() consistent with doc - #13841
Conversation
vsemozhetbyt
commented
Jun 21, 2017
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
There was a problem hiding this comment.
Would it make more sense to do this on line 151?
There was a problem hiding this comment.
Is it OK it would be worthless to do all this if debugs[set] is already true?
There was a problem hiding this comment.
Maybe we should place all the process.env.NODE_DEBUG processing in the if clause?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
If you mean making debugEnviron a Set, then yes, that works for me.
There was a problem hiding this comment.
Why the uppercase variable name?
There was a problem hiding this comment.
To differ from section and to clarify the difference. What would you propose? Just reassign section? Or name it like sectionUpperCase?
There was a problem hiding this comment.
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.
@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
2 unstable results due to flaky |
So will we backport it or consider it semver-major? |
cjihrig
commented
Jun 21, 2017
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. |
I've set semver major for now, please, remove and backport if it will be reconsidered. |
jasnell
left a comment
There was a problem hiding this comment.
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
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
@jasnell Hopefully done. |
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: #13841Fixes: #13728 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
vsemozhetbyt
commented
Jun 23, 2017
Landed in 3b0e800 |
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
util
Fixes: #13728