Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.2k
doc: add documentation for deprecation properties#16539
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
refack commented Oct 27, 2017
Code LGTM, but the question is should they be documented or removed. AFAICT they were exposed as a rough mechanism of communicating the CLI flags to JS, they are not necessarily part of a succinct API. Alternatively they (and other CLI flag mappings) could go behind a new |
doc/api/process.md Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A nit: --trace-deprecation -> --throw-deprecation
doc/api/process.md Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems this should be placed after the process.title, ABC-wise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my bad -- don't know why I thought a ### about IO was a ## 😞
vsemozhetbyt commented Oct 27, 2017 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
I am not sure about some things.
console.log(process.noDeprecation,process.throwDeprecation,process.traceDeprecation);undefined undefined undefined
Let's see what other collaborators think. |
vsemozhetbyt commented Oct 27, 2017
@refack It seems we cannot just remove them as they have been documented in |
vsemozhetbyt commented Oct 27, 2017
BTW, thank you for the PR and digging into the history! |
addaleax commented Oct 27, 2017
Like |
refack commented Oct 27, 2017
Hence my ambivalence about fully embracing those flags as 1st class API citizens 😕 |
BridgeAR commented Nov 23, 2017
I personally think it would be best to deprecate these instead of documenting them. At the same time we can rephrase the util part where they are mentioned. |
maclover7 commented Nov 28, 2017
@BridgeAR Even if the props are deprecated publicly, they are still used within parts of Node core (like lib/internal/process/warning.js for example), so I'm not sold on what the maintenance benefit would be for doing that :( |
BridgeAR commented Dec 7, 2017
@maclover7 even if we use some internally it does not mean users should always use those things as well. That is what |
nylen commented Dec 26, 2017 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Please document and standardize these properties. I want to use Edit: See also #17871 |
doc/api/process.md Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly change "returns if" to "indicates whether or not"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would go with just "indicates whether", without the "or not".
doc/api/process.md Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here and below.
maclover7 commented Dec 31, 2017
updated @cjihrig@apapirovski |
maclover7 commented Jan 2, 2018
ping @cjihrig@apapirovski would like to try and land this |
cjihrig left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes themselves look fine, but there were a few comments by others that brought up the alternative of deprecating and removing these properties.
maclover7 commented Jan 2, 2018
maclover7 commented Jan 2, 2018
Going to open a followup issue to discuss possible deprecation/removal of the properties. |
maclover7 commented Jan 13, 2018
PR-URL: #16539Fixes: #16394 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: #16539Fixes: #16394 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: #16539Fixes: #16394 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: #16539Fixes: #16394 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Refs: #16394
Relevant commit history:
node: Add --throw-deprecationAdd --no-deprecation and --trace-deprecation flagscc @vsemozhetbyt, I tried to avoid duplicating tons of documentation by linking out to other sections of the process docs, would love your thoughts :)
Checklist
Affected core subsystem(s)
doc