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

doc: add a sentence about REPLACEME in code changes #25961

Closed
wants to merge 2 commits into from

Conversation

lance
Copy link
Member

@lance lance commented Feb 6, 2019

When adding to the REPL API recently, I was unsure what to put as a
version number for the new method I added. This change adds a reference
to releases.md where the use of REPLACEME is discussed.

Checklist

When adding to the REPL API recently, I was unsure what to put as a
version number for the new method I added. This change adds a reference
to `releases.md` where the use of `REPLACEME` is discussed.
@lance lance added the doc Issues and PRs related to the documentations. label Feb 6, 2019
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 6, 2019
@lance
Copy link
Member Author

lance commented Feb 6, 2019

This is a simple doc change. Any reason not to fast-track it?

@@ -116,7 +116,9 @@ time to ensure that the changes follow the Node.js code style guide.
Any documentation you write (including code comments and API documentation)
should follow the [Style Guide](../../STYLE_GUIDE.md). Code samples included
in the API docs will also be checked when running `make lint` (or
`vcbuild.bat lint` on Windows).
`vcbuild.bat lint` on Windows). If you are adding to or deprecating the API,
be sure to use `REPLACEME` for the version numbers as specified in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
be sure to use `REPLACEME` for the version numbers as specified in
use `REPLACEME` for the version number as specified in

@Trott
Copy link
Member

Trott commented Feb 6, 2019

To be honest, this sentence seems to come out of nowhere. There's no context explaining the YAML stuff etc. This might be more confusing than enlightening if a new person is reading it and doesn't already know about the YAML stuff. That said, having it documented is an improvement over not having it documented, so I'm totally OK with this landing as-is. But if there's a place to move this where it would have more context, that would be good. (Again, though, someone else can do that in a subsequent PR if the only goal here is to have it documented somewhere that a contributor might find it.)

An idea: Would it make sense to instead put this as a YAML comment in every doc/api/*.md file? That would put the information right where someone needs it at the time that they need it. On the other hand, that's a lot of copy/pasted text....
¯\(ツ)

@@ -116,7 +116,9 @@ time to ensure that the changes follow the Node.js code style guide.
Any documentation you write (including code comments and API documentation)
should follow the [Style Guide](../../STYLE_GUIDE.md). Code samples included
in the API docs will also be checked when running `make lint` (or
`vcbuild.bat lint` on Windows).
`vcbuild.bat lint` on Windows). If you are adding to or deprecating the API,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`vcbuild.bat lint` on Windows). If you are adding to or deprecating the API,
`vcbuild.bat lint` on Windows). If you are adding to or deprecating an API,

@Trott
Copy link
Member

Trott commented Feb 6, 2019

Maybe the only context that needs to be added is just a mention that this is referring to the YAML metadata?

If you are adding to or deprecating an API, use `REPLACEME`
for the version number in the documentation YAML metadata.

Probably no need to link to releases.md in that case?

@lance
Copy link
Member Author

lance commented Feb 6, 2019

@Trott I don't think it would be that hard to add it as a comment in the YAML itself. E.g.

sed -i 's/^<!-- YAML*$/<!-- YAML\n# When adding to or deprecating an API, use \'REPLACEME\' for the version number/' doc/api/*.md

@richardlau
Copy link
Member

An idea: Would it make sense to instead put this as a YAML comment in every doc/api/*.md file? That would put the information right where someone needs it at the time that they need it. On the other hand, that's a lot of copy/pasted text....
¯_(ツ)_/¯

My only concern here is whether the REPLACEME in the comment would also get replaced when a release was cut.

@Trott
Copy link
Member

Trott commented Feb 6, 2019

I think this is fast-track-able in its current form. Please 👍 here to approve fast-tracking.

@Trott Trott added the fast-track PRs that do not need to wait for 48 hours to land. label Feb 6, 2019
@lance
Copy link
Member Author

lance commented Feb 7, 2019

@Trott I inadvertently pushed this branch to nodejs/node instead of my fork. Not sure of the best way to deal with this, as I would rather not close this PR and start over. I am assuming that I can just delete the branch here on upstream once the PR is merged. Please advise if this is not the best route.

@Trott
Copy link
Member

Trott commented Feb 7, 2019

I am assuming that I can just delete the branch here on upstream once the PR is merged. Please advise if this is not the best route.

That seems like it would be fine to me.

@danbev
Copy link
Contributor

danbev commented Feb 7, 2019

Landed in 93a57c3.

@danbev danbev closed this Feb 7, 2019
danbev pushed a commit that referenced this pull request Feb 7, 2019
When adding to the REPL API recently, I was unsure what to put as a
version number for the new method I added. This change adds a reference
to `releases.md` where the use of `REPLACEME` is discussed.

PR-URL: #25961
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@Trott Trott deleted the pull-request-doc-updates branch February 7, 2019 05:16
addaleax pushed a commit that referenced this pull request Feb 7, 2019
When adding to the REPL API recently, I was unsure what to put as a
version number for the new method I added. This change adds a reference
to `releases.md` where the use of `REPLACEME` is discussed.

PR-URL: #25961
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@targos targos mentioned this pull request Feb 14, 2019
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. doc Issues and PRs related to the documentations. fast-track PRs that do not need to wait for 48 hours to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants