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

[JENKINS-73744] Tweaks to Node.js contributing instructions + renovate fix #9757

Merged
merged 9 commits into from
Sep 21, 2024

Conversation

scherler
Copy link
Contributor

@scherler scherler commented Sep 19, 2024

The PR #9718 introduced suboptimal wording and forgot to update .github/renovate.json originally addressed
JENKINS-73744.

Testing done

Proposed changelog entries

  • human-readable text

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

Signed-off-by: Thorsten Scherler <scherler@gmail.com>
.gitignore Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
Co-authored-by: Tim Jacomb <21194782+timja@users.noreply.github.com>
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
Copy link
Member

@basil basil left a 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 <scherler@gmail.com>
@scherler
Copy link
Contributor Author

scherler commented Sep 20, 2024

@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. 🤷‍♂️

.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
Co-authored-by: Tim Jacomb <21194782+timja@users.noreply.github.com>
CONTRIBUTING.md Outdated Show resolved Hide resolved
Co-authored-by: Tim Jacomb <21194782+timja@users.noreply.github.com>
@timja timja requested a review from basil September 20, 2024 07:58
@timja timja added the skip-changelog Should not be shown in the changelog label Sep 20, 2024
@timja timja changed the title [JENKINS-73744_follow_up] add npm lock to ignore [JENKINS-73744] Tweaks to Node.js contributing instructions Sep 20, 2024
@scherler scherler changed the title [JENKINS-73744] Tweaks to Node.js contributing instructions [JENKINS-73744] follow up: update docu and renovate config Sep 20, 2024
@scherler
Copy link
Contributor Author

@timja
image
Seems we changed at the same time, I do not care about mine but I did changed the original title not yours (at least not knowingly)

@timja timja changed the title [JENKINS-73744] follow up: update docu and renovate config [JENKINS-73744] Tweaks to Node.js contributing instructions + renovate fix Sep 20, 2024
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.
Copy link
Member

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 Corepack corepack 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?

Copy link
Contributor Author

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

Copy link
Contributor Author

@scherler scherler Sep 20, 2024

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

Copy link
Member

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 Corepack corepack 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").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for explaining!

CONTRIBUTING.md Outdated Show resolved Hide resolved
Co-authored-by: Tim Jacomb <21194782+timja@users.noreply.github.com>
Copy link
Member

@basil basil left a 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!

@timja timja merged commit 10154f6 into jenkinsci:master Sep 21, 2024
16 checks passed
@scherler scherler deleted the JENKINS-73744_follow_up branch September 23, 2024 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog Should not be shown in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants