-
-
Notifications
You must be signed in to change notification settings - Fork 33.8k
doc: update deprecations info in COLLABORATOR_GUIDE #20523
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
Conversation
|
As much as I don't want to bikeshed this if I can avoid it, I certainly don't want it to slip in unnoticed, so: @nodejs/tsc (Also adding the |
|
Pushed a couple of minor fixes (corrected a label name, added a link to the documentatino for a CLI option). New Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite/663/ |
COLLABORATOR_GUIDE.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.
Should we as well refer to doc/api/cli.md#node_pending_deprecation1 (environment variable section) here?
BridgeAR
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.
There were multiple disputes with collaborators about all the information that is now removed. So I would definitely not want to remove any of the current information but I agree that the wording here is nicer for some parts.
COLLABORATOR_GUIDE.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.
I think it would be good to keep this. Especially non-native English speakers and people who are new to coding might not know what a deprecation stands for.
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 agree with @BridgeAR.
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.
PR to improve Deprecation definition was posted at #20788
COLLABORATOR_GUIDE.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.
The semver part is important for us collaborators since it is otherwise not clear that a doc only deprecation may be semver-minor. I would be +1 for moving the notice up to the description of doc only deprecation.
COLLABORATOR_GUIDE.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.
I am relatively certain that this came quite a while ago as well and it was added on purpose. So I would like to keep it to prevent confusion for collaborators.
COLLABORATOR_GUIDE.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.
IMO this should be kept in here.
COLLABORATOR_GUIDE.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.
I think it is good to keep it as as. This makes it clear to the reader that we try to report everything we find but we can not always handle everything. It also helps collaborators to know that this is something they should do when opening a PR.
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 think it is good to keep it as as.
This is the paragraph most in need of removal IMO.
This makes it clear to the reader that we try to report everything we find but we can not always handle everything.
If we're trying to communicate that to the community, then the COLLABORATOR_GUIDE.md is the wrong place to do it. Information aimed at end users should be in deprecations.md instead.
Perhaps more importantly, I take issue with the idea that this paragraph makes anything clear. It is almost exclusively weasel words. The very first sentence starts with: "A best effort will be made..." What does "best effort" mean in this context? (Answer: It is meaningless. It seems significant but actually indicates nothing specific. Does that mean the Node.js Twitter will tweet out the information, for example? Maybe. No one can say. It could mean anything. So it means nothing.)
The paragraph appears to contradict itself on timing too. At the very least, the juxtaposition of "as soon as possible" with "preferably" muddies things. (Again, it all sounds like it's saying something significant but is saying nothing specific or meaningful IMO.)
If deprecations need to be disclosed to the community via one or more specific channels that Collaborators need to be aware of and responsible for, all within certain timeframes, then let's put that information in the doc. But that information does not exist and will take time to hash out. As it stands, we have a paragraph of meaningless platitudes directed at people who aren't even going to read this doc but are likely to read a different doc. Let's remove it.
It also helps collaborators to know that this is something they should do when opening a PR.
That "something they should do" is the only specific information in the paragraph and it's why I added this sentence to the doc, which is better than the existing text IMO because it clearly explains how to do the thing you're supposed to do:
Use the `notable-change` label on all pull requests that add a new deprecation
or move an existing deprecation to a new deprecation level.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.
For me the first sentence seems pretty important. I understand the something should be done part as a request to check CITGM and to open PRs in userland modules to fix the upcoming issues. This is done with a "best effort" and we can not guarantee anything and decide if it makes sense to do it or not.
COLLABORATOR_GUIDE.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.
I agree with @BridgeAR.
COLLABORATOR_GUIDE.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.
I would prefer that the semver level of the deprecation is either in the previous list or under another heading. This is important, and it does not get enough visibility here.
COLLABORATOR_GUIDE.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.
For me the first sentence seems pretty important. I understand the something should be done part as a request to check CITGM and to open PRs in userland modules to fix the upcoming issues. This is done with a "best effort" and we can not guarantee anything and decide if it makes sense to do it or not.
COLLABORATOR_GUIDE.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.
When being on this: we might want to reword this that the backward-incompatible change should be done when moving something to EOL.
|
@Trott is this still WIP? The deprecation information has been changed in other PRs in the meanwhile and I am not sure if you are still working on this. |
|
@BridgeAR Yes, still WIP. Sorry about the delay. I'm glad the definition part got sorted, but I still have to resolve the comments on the rest of it. |
f8a9de3 to
3ceccdc
Compare
The deprecations section in the COLLABORATOR_GUIDE is a bit long-winded and repetitive. It also includes some aspirational items that appear worded to create "an impression that a specific or meaningful statement has been made, when instead only a vague or ambiguous claim has actually been communicated." (Putting it in quotes because I'm copying it from Wikipedia.) This commit edits the text to make it more concise and meaningful.
The deprecations section in the COLLABORATOR_GUIDE is a bit long-winded
and repetitive. It also includes some aspirational items that appear
worded to create "an impression that a specific or meaningful statement
has been made, when instead only a vague or ambiguous claim has actually
been communicated." (Putting it in quotes because I'm copying it from
Wikipedia.)
This commit edits the text to make it more concise and meaningful.
Checklist