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

lint: update a name that wasn't capitalized #340

Merged
merged 4 commits into from
Feb 15, 2022

Conversation

mahmoud-moursy
Copy link
Contributor

I've found some docs that don't capitalize the first letter of Valgrind (namely the contributor doc related to investigating native memory leaks). This additional lint should help to fix that grammatical issue.

I've found some docs that don't capitalize the first letter of Valgrind (namely the contributor doc related to investigating native memory leaks). This additional lint should help to fix that grammatical issue.
index.js Outdated Show resolved Hide resolved
Thanks, @nschonni!

Co-authored-by: Nick Schonning <nschonni@gmail.com>
index.js Outdated Show resolved Hide resolved
@nschonni
Copy link
Member

You'll need a corresponding PR in the other repo to clean it up, before the CI here will go green

@mahmoud-moursy
Copy link
Contributor Author

Already on it!

@mahmoud-moursy
Copy link
Contributor Author

Finished this a while ago but forgot to link the PR

index.js Outdated
@@ -102,6 +102,7 @@ const plugins = [
{ no: "[Rr][Ff][Cc]\\d+", yes: "RFC <number>" },
{ yes: "Unix" },
{ yes: "V8" },
{ no: "valgrind", yes: "Valgrind" },
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the PR. Two comments:

  1. We don't need the no entry in this case. Just the yes is enough (and will prevent people from doing other things like ValGrind or VALGRIND).
  2. The entries are in alphabetical order. Can you move this one up one line (so it's after Unix and before V8)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can do, and will do! :)

- Update index.js to remove unnecessary `no` clause
- Move it to alphabetical order
@Trott Trott merged commit a022be7 into nodejs:main Feb 15, 2022
@Trott
Copy link
Member

Trott commented Feb 15, 2022

Whoops, I should have squashed rather than rebased. Going to force push to fix that.

@Trott
Copy link
Member

Trott commented Feb 15, 2022

Thanks for the contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants