-
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: Clarify msg when doc/api/cli.md not updated #10872
Conversation
Maybe worth including either a link to the docs or the command to run, i.e. one of: perl -pi -e "s/REPLACEME/$VERSION/g" doc/api/*.md
sed -i "s/REPLACEME/$VERSION/g" doc/api/*.md Also
Otherwise LGTM |
Yeah can do - it's just a bit more difficult to write the message in 80 characters and include all the references. I've changed it from:
to
|
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 with a nit.
@@ -498,7 +498,7 @@ PKGDIR=out/dist-osx | |||
release-only: | |||
@if [ "$(DISTTYPE)" != "nightly" ] && [ "$(DISTTYPE)" != "next-nightly" ] && \ | |||
`grep -q REPLACEME doc/api/*.md`; then \ | |||
echo 'Please update Added: tags in the documentation first.' ; \ | |||
echo 'Please update REPLACEME in Added: tags in doc/api/*.md (See doc/releases.md)' ; \ |
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.
s/See/see/
CI: https://ci.nodejs.org/job/node-test-pull-request/6182/console btw we need CI when the change is on the makefile? |
@italoacasas Not sure we really need CI, but we might as well run it anyway. |
PR-URL: #10872 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Italo A. Casas <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: James M Snell <[email protected]>
Landed in 2a295bc |
PR-URL: #10872 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Italo A. Casas <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#10872 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Italo A. Casas <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#10872 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Italo A. Casas <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #10872 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Italo A. Casas <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #10872 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Italo A. Casas <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #10872 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Italo A. Casas <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #10872 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Italo A. Casas <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: James M Snell <[email protected]>
Checklist
Affected core subsystem(s)
build
The message printed out when the doc/api/cli.md file had not had the REPLACEME tags replaced could do with being made clearer. This PR updates it to indicate which the top level Makefile is checking for.
Cc: @bnoordhuis who put in the original message.