-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
[JENKINS-73744] Tweaks to Node.js contributing instructions + renovate fix #9757
Conversation
Signed-off-by: Thorsten Scherler <[email protected]>
Co-authored-by: Tim Jacomb <[email protected]>
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.
In the second portion of this document, there are numerous occurrences of the phrase "adding Node and Yarn to your path" linking to the #running-the-yarn-frontend-build
section of the documentation. These phrases are now out-of-date, so I suggest changing them to "optionally adding Node and Yarn to your path".
Signed-off-by: Thorsten Scherler <[email protected]>
@basil I changed the wording now as you requested. In hindsight we may have saved numerous interactions (and time) if you would have suggested the wording to use in the first place since you seem to had strong feelings about it. 🤷♂️ |
Co-authored-by: Tim Jacomb <[email protected]>
Co-authored-by: Tim Jacomb <[email protected]>
@timja |
CONTRIBUTING.md
Outdated
@@ -52,6 +52,9 @@ MAVEN_OPTS='--add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/ja | |||
|
|||
### Running the Yarn frontend build | |||
|
|||
> [!TIP] | |||
> If you already have Node.js installed, you do not need to change your path. Start using `yarn` by enabling [Corepack](https://yarnpkg.com/corepack) `corepack enable`, if it isn't already; this will add the yarn binary to your PATH. |
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.
Start using
yarn
by enabling Corepackcorepack enable
Should there be a preposition before corepack enable
?
Start using
yarn
[…]; this will add the yarn binary […].
Why is the reference to Yarn in monospaced text at the beginning of the sentence but not at the end of the sentence?
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.
yarn
is a command which is lowercase. ...but please use the suggest function in github
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 not a native speaker and the above is adopted from https://yarnpkg.com/getting-started/install
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 above is adopted from https://yarnpkg.com/getting-started/install
For reference, the original material reads:
Start by enabling Corepack, if it isn't already; this will add the
yarn
binary to your PATH:
corepack enable
The original material correctly uses a colon to separate corepack enable
from the beginning of the sentence. In contrast, the material in this PR does not separate corepack enable
(which functions as a noun) from the sentence in any way:
Start using
yarn
by enabling Corepackcorepack enable
Here Corepack is the object of "enabling", and corepack enable
(functioning as a noun) is left dangling.
One way to correctly use corepack enable
as a noun in the middle of the sentence would be as an object of the preposition "with" (e.g., "with corepack enable
").
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.
Thanks for explaining!
Co-authored-by: Tim Jacomb <[email protected]>
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.
Thanks for the PR!
The PR #9718 introduced suboptimal wording and forgot to update
.github/renovate.json
originally addressedJENKINS-73744.
Testing done
Proposed changelog entries
Proposed upgrade guidelines
N/A
Submitter checklist
Desired reviewers
@mention
Before the changes are marked as
ready-for-merge
:Maintainer checklist