-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
doc: revise Introducing New Modules #25975
Conversation
Revise "Introducing New Modules" in the Collaborator Guide: * Improve clarity. * Remove passive voice. * Keep sentences short for ease of reading/scanning.
@@ -292,25 +292,23 @@ metadata. Raise a Pull Request like any other change. | |||
|
|||
### Introducing New Modules | |||
|
|||
Semver-minor commits that introduce new core modules should be treated with | |||
extra care. | |||
Treat commits that introduce new core modules with extra care. |
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.
Is it mentioned elsewhere that introducing a new core module is semver-minor?
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.
Not as far as I know. If it's desirable to have it here, I can add a sentence. I'd prefer to leave it out as it's self-evident and low-value (as someone else will certainly add the label if the author does not) and this doc is plenty long as it is. But I don't have a strong preference, so I'll yield if a Collaborator thinks it needs to be in there.
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 don't really have a strong preference either. I guess if we do mention it it can go in the bullet list below (probably next to "Label with the tsc-review
label.").
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.
By definition it adds functionality, ergo, semver-minor
.
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.
LGTM
Revise "Introducing New Modules" in the Collaborator Guide: * Improve clarity. * Remove passive voice. * Keep sentences short for ease of reading/scanning. PR-URL: nodejs#25975 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Landed in 016c7b9 |
Revise "Introducing New Modules" in the Collaborator Guide: * Improve clarity. * Remove passive voice. * Keep sentences short for ease of reading/scanning. PR-URL: #25975 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Revise "Introducing New Modules" in the Collaborator Guide:
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes