-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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: Update CONTRIBUTING.MD to link to V8 guide #10070
Conversation
nit: can you add the subsystem to the commit message. |
@@ -15,6 +15,10 @@ Node.js software. | |||
For general help using Node.js, please file an issue at the | |||
[Node.js help repository](https://github.com/nodejs/help/issues). | |||
|
|||
For help working with the vendored version of V8 included in Node's dependency |
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.
This guide is specifically about how to properly update V8. I think the language here should be specific about that
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.
Very tiny nit, but when referring to the project, it's Node.js
and not Node
. (And when referring to the executable, it's node
and not Node
.)
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.
@thealphanerd Do you have a recommendation as to what exactly the text perhaps should be?
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 instructions on updating the version of V8 included in node's dependency directory...
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.
@sarahmeyer Does the text suggested by @thealphanerd seem correct and otherwise effective to you? If so, would you be willing to update this PR to use it or some variation on it?
ping @sarahmeyer :-) |
Currently, two of the guides in the `/doc/guides` directory are actually guides for working on the Node project. Of those, one is linked from this page. This change adds a note to point people to the other.
In the interest of getting this small-but-meaningful change through the process, I updated the text myself and included a few other minor fixes to some surrounding text in the doc. @sarahmeyer Hope that was OK for me to do! @thealphanerd, LGTY? |
lgtm |
Contributor indicated 'lgtm' in subsequent comment after changes were made to address the issues raised in the review.
Currently, two of the guides in the `/doc/guides` directory are actually guides for working on the Nodei.js project. Of those, one is linked from this page. This change adds a note to point people to the other. PR-URL: nodejs#10070 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>
Landed in c732bd1. |
Currently, two of the guides in the `/doc/guides` directory are actually guides for working on the Nodei.js project. Of those, one is linked from this page. This change adds a note to point people to the other. PR-URL: #10070 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>
Currently, two of the guides in the `/doc/guides` directory are actually guides for working on the Nodei.js project. Of those, one is linked from this page. This change adds a note to point people to the other. PR-URL: #10070 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>
Currently, two of the guides in the `/doc/guides` directory are actually guides for working on the Nodei.js project. Of those, one is linked from this page. This change adds a note to point people to the other. PR-URL: #10070 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>
Currently, two of the guides in the `/doc/guides` directory are actually guides for working on the Nodei.js project. Of those, one is linked from this page. This change adds a note to point people to the other. PR-URL: #10070 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>
Currently, two of the guides in the `/doc/guides` directory are actually guides for working on the Nodei.js project. Of those, one is linked from this page. This change adds a note to point people to the other. PR-URL: #10070 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>
Currently, two of the guides in the `/doc/guides` directory are actually guides for working on the Nodei.js project. Of those, one is linked from this page. This change adds a note to point people to the other. PR-URL: #10070 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>
Checklist
Affected core subsystem(s)
doc
Description of change
Currently, two of the guides in the
/doc/guides
directory are actually guides for working on the Node project. Of those, one is linked from this page. This change adds a note to point people to the other.