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

process: refactor emitWarning by BridgeAR · Pull Request #20726 · nodejs/node · GitHub

/ node Public

process: refactor emitWarning - #20726

Closed
BridgeAR wants to merge 1 commit into
nodejs:masterfrom
BridgeAR:improve-warnings
ConversationCommits (1)ChecksFiles changed
Closed

process: refactor emitWarning#20726
BridgeAR wants to merge 1 commit into
nodejs:masterfrom
BridgeAR:improve-warnings

Conversation

BridgeAR commented May 14, 2018

Copy link
Copy Markdown
Member

Currently the code does a couple of checks to often even though they
are not necessary.

Also remove a double whitespace in a error comment.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

nodejs-github-bot added the process Issues and PRs related to the process subsystem. label May 14, 2018

BridgeAR commented May 14, 2018

Copy link
Copy Markdown
Member Author

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

apapirovski reviewed May 14, 2018
View reviewed changes

apapirovski left a comment

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

There are changes here that change how the code functions. I'm -1 on those happening. What would be left after reverting those seems fine to me.

Comment thread lib/internal/process/warning.js Outdated

apapirovski May 14, 2018

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

I don't think this is the same. If type is an object but type.type is truthy and not a string then this won't throw anymore.

BridgeAR May 17, 2018

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

Addressed.

Comment thread lib/internal/process/warning.js Outdated

apapirovski May 14, 2018

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

This removes the possibility of an undefined warning that has the other properties specified.

BridgeAR May 14, 2018

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

That is not officially supported.

apapirovski May 14, 2018 •
edited
Loading

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

While not documented, as far as I can tell this was requested in #4782

It has also worked this way for two years... I don't know what usage people have for this or what they're passing to it. It might very well be a breaking change.

jasnell May 15, 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.

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

I agree with @apapirovski on this. This is a breaking change even if not properly documented.

BridgeAR May 17, 2018 •
edited
Loading

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

process.emitWarning() produces: (node:XXXX) Warning. I am not convinced that this is useful at all. This seems like a bugfix as no args should result in an error. Therefore I would say it is a patch. However, I would also keep this if it makes this semver-major.

Ping @nodejs/tsc please also give an opinion.

Trott May 17, 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.

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

process.emitWarning() produces: (node:XXXX) Warning. I am not convinced that this is useful at all.

process.emitWarning(undefined, 'fhqwhgads', 'come-on') produces the slightly more useful (node:91812) [come-on] fhqwhgads.

I have no idea if anyone uses that pattern or not. Maybe it's time for me to figure out how to use Gzemnid.

In the spirit of the onboarding-extras.md:

be conservative – that is, if a change has the remote chance of breaking something, go for semver-major

It seems like this should be semver-major as it has a "remote change of breaking something".

(If we're not going to call this semver-major, I won't push back, but we really ought to remove the quoted sentence above from the doc since it is clearly not something we abide by in that case.)

BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 18, 2018

BridgeAR commented May 18, 2018

Copy link
Copy Markdown
Member Author

I marked it as semver-major. PTAL.

mcollina commented May 21, 2018

Copy link
Copy Markdown
Member

I would expect to see more tests changed as it's semver-major. Can you add some more test that verify the change in behavior?

mcollina approved these changes Jun 2, 2018
View reviewed changes

mcollina 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

LGTM

BridgeAR requested a review from a team June 11, 2018 01:17

BridgeAR commented Jun 11, 2018

Copy link
Copy Markdown
Member Author

Rebased due to conflicts. I also added a test case for the changed behavior as requested.

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

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jun 11, 2018

TimothyGu commented Jun 13, 2018

Copy link
Copy Markdown
Member

I agree with @apapirovski. This PR was "marketed" as a simple refactoring by its title… and it should stay this way at least for this PR, or we could change the title to make it clear that we are changing the behavior.

BridgeAR changed the title process: reduce possible code paths by using else process: refactor emitWarning Jun 25, 2018
process: refactor emitWarning
10562c2 
Remove a couple of obsolete checks by refactoring the code with else.
Also remove the possibility of an undefined warning. That is neither
documented nor does it result in a usable warning.

BridgeAR commented Jun 25, 2018

Copy link
Copy Markdown
Member Author

@TimothyGu @apapirovski I changed the description of the PR accordingly.

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

mcollina approved these changes Jun 25, 2018
View reviewed changes

mcollina 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

Still LGTM

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jul 16, 2018
process: refactor emitWarning
49681e7 
Remove a couple of obsolete checks by refactoring the code with else.
Also remove the possibility of an undefined warning. That is neither
documented nor does it result in a usable warning.

PR-URL: nodejs#20726
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

BridgeAR commented Jul 16, 2018

Copy link
Copy Markdown
Member Author

CI https://ci.nodejs.org/job/node-test-pull-request/15892/ (passed)

Landed in 49681e7 🎉

BridgeAR closed this Jul 16, 2018
BridgeAR deleted the improve-warnings branch January 20, 2020 11:34
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

Trott Trott left review comments
mcollina mcollina approved these changes
jasnell jasnell approved these changes
+1 more reviewer
apapirovski apapirovski left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. process Issues and PRs related to the process subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL