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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

process: suggest --trace-warnings when printing warning #32797

Closed
wants to merge 3 commits into from

Conversation

addaleax
Copy link
Member

Suggest using --trace-warnings or --trace-deprecation the first
time a warning is emitted without a stack trace, similar to how
we suggest --trace-uncaught when printing uncaught exceptions
without a stack trace.

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

Suggest using `--trace-warnings` or `--trace-deprecation` the first
time a warning is emitted without a stack trace, similar to how
we suggest `--trace-uncaught` when printing uncaught exceptions
without a stack trace.
@nodejs-github-bot nodejs-github-bot added the process Issues and PRs related to the process subsystem. label Apr 12, 2020
addaleax added a commit to addaleax/node that referenced this pull request Apr 12, 2020
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 12, 2020

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

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 13, 2020
@addaleax
Copy link
Member Author

Landed in 907ebdd

addaleax added a commit that referenced this pull request Apr 15, 2020
Suggest using `--trace-warnings` or `--trace-deprecation` the first
time a warning is emitted without a stack trace, similar to how
we suggest `--trace-uncaught` when printing uncaught exceptions
without a stack trace.

PR-URL: #32797
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@addaleax addaleax closed this Apr 15, 2020
@addaleax addaleax deleted the warning-trace branch April 15, 2020 00:11
addaleax added a commit that referenced this pull request Apr 15, 2020
MylesBorins pushed a commit that referenced this pull request Apr 17, 2020
Suggest using `--trace-warnings` or `--trace-deprecation` the first
time a warning is emitted without a stack trace, similar to how
we suggest `--trace-uncaught` when printing uncaught exceptions
without a stack trace.

PR-URL: #32797
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 17, 2020
@targos targos added baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v12.x labels Apr 25, 2020
BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
Suggest using `--trace-warnings` or `--trace-deprecation` the first
time a warning is emitted without a stack trace, similar to how
we suggest `--trace-uncaught` when printing uncaught exceptions
without a stack trace.

PR-URL: #32797
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
@BridgeAR BridgeAR mentioned this pull request Apr 28, 2020
codebytere pushed a commit that referenced this pull request Jun 7, 2020
@codebytere
Copy link
Member

@addaleax should this go back into v12.x?

@addaleax
Copy link
Member Author

addaleax commented Jun 9, 2020

@codebytere I’m not sure myself.

@targos @cjihrig @devnexen @richardlau @BridgeAR You approved this, wdyt?

@cjihrig
Copy link
Contributor

cjihrig commented Jun 10, 2020

I don't have a strong opinion one way or the other, but I would not backport this unless it starts causing a lot of problems backporting other commits.

@BridgeAR
Copy link
Member

I would backport it. Otherwise this is going to cause conflicts on each warning that is backported and it is helpful for users.

@richardlau
Copy link
Member

I don't have a strong opinion one way or the other, but I would not backport this unless it starts causing a lot of problems backporting other commits.

I agree with @cjihrig 's position.

I do think the extra message is useful but it does introduce an extra line of output which, as can be seen by the test changes, can affect anything trying to parse the output of running a Node.js application.

If the change breaks anything in CITGM for 12.x I'd be a hard no to backporting.

@BridgeAR BridgeAR added dont-land-on-v12.x and removed baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v12.x labels Jun 10, 2020
@BridgeAR
Copy link
Member

I added the don't land on 12 label due to the comments above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants