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.

Sign up for GitHub

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

Jump to bottom

doc: fix typo in COLLABORATOR_GUIDE.md #20742

Closed
wants to merge 2 commits into from
Closed

doc: fix typo in COLLABORATOR_GUIDE.md #20742

wants to merge 2 commits into from

Conversation

vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented May 15, 2018 •
edited
Loading

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

Refs: #20740

@vsemozhetbyt vsemozhetbyt requested a review from a team as a code owner May 15, 2018 08:39
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label May 15, 2018
@vsemozhetbyt
Copy link
Contributor Author

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite/710/

@vsemozhetbyt
Copy link
Contributor Author

cc @nodejs/linting How can we fix the linting issue in this case?

@BridgeAR
Copy link
Member

@vsemozhetbyt good question... I guess it would actually be best to rename the group instead.

Ping @nodejs/tsc

@ofrobots
Copy link
Contributor

{digression} IMHO, this doesn't need review from the TSC or even a ping to the list of all collaborators – this is an uncontroversial typo fix. Global pings should be avoided unless necessary. {/digression}

mhdawson
mhdawson approved these changes May 15, 2018
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@Trott
Copy link
Member

Trott commented May 15, 2018

How about we change the capitalization on the group name instead (and in the CODEOWNERS file too)? Then the linter will be happy and we'll have a group name that correctly styles the name V8. We've tried to use V8 everywhere to avoid confusion about v being for version and also just to be consistent with Google's styling for the JavaScript engine.

@Trott
Copy link
Member

Trott commented May 15, 2018

(@ofrobots I think the ping may have been automated by the CODEOWNERS file. It's possible we don't need a TSC ping for changes to COLLABORATOR_GUIDE. Basically only if it entails a governance change. Maybe that line can be removed from CODEOWNERS.)

@Trott
Copy link
Member

Trott commented May 15, 2018

I'd prefer we change the name of the team to use V8.

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented May 15, 2018 •
edited
Loading

@ofrobots It seems the ping was not about typo itself: we have a conflict between linting rule requiring V8 case and the team name with v8. If we land it this way, we will have constant linting failures. If we leave it out, we will have broken team tag.

@Trott
Copy link
Member

Trott commented May 15, 2018

@vsemozhetbyt Are you 👍if I change the team name to use V8 instead of v8? Then we can close this PR and you (or I or anyone else) can update the CODEOWNERS file?

@vsemozhetbyt
Copy link
Contributor Author

@Trott Yes, please, this seems more consistent.

@Trott
Copy link
Member

Trott commented May 15, 2018

It appears that GitHub has started (or maybe always has?) lowercasing team names in @-mentions regardless of how you actually name the team.

I'll see about updating the prohibited-strings code to ignore prohibited strings that appear inside of @-strings so this starts passing.

@Trott
Copy link
Member

Trott commented May 15, 2018

Published a new version of remark-lint-prohibited-strings to avoid the false positive here. Unfortunately, people will likely need to do make lint-md-clean && make lint-md-build after this lands to avoid getting the lint error.

I put the update as the first commit on this PR with the original changes as the second commit. (I recommend leaving them separate for what I hope are obvious reasons.)

Full CI (because now we're updating tools): https://ci.nodejs.org/job/node-test-pull-request/14895/

@vsemozhetbyt Thanks for your patience with this!

@Trott
Copy link
Member

Trott commented May 15, 2018

Hmmm...looks like I need to remove some OS-dependent stuff:

17:41:03 npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@1.2.4 (node_modules/fsevents):
17:41:03 npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for fsevents@1.2.4: wanted {"os":"darwin","arch":"any"} (current: {"os":"freebsd","arch":"x64"})

Trott and others added 2 commits May 15, 2018 14:47
@Trott
tools: update prohibited-strings md linting
d231049 
Update to remark-lint-prohibited-strings 1.0.3 to avoid false positives
with GitHub team names which must be lowercased no matter what our
prohibited-strings rules say about a particular string.
@vsemozhetbyt @Trott
doc: fix typo in COLLABORATOR_GUIDE.md
265b97c 
Refs: #20740
@Trott
Copy link
Member

Trott commented May 15, 2018

CI: https://ci.nodejs.org/job/node-test-commit/18517/

@Trott
Copy link
Member

Trott commented May 15, 2018

Re-running arm-fanned, addons failure is clearly spurious: https://ci.nodejs.org/job/node-test-commit-arm-fanned/1230/

@Trott
Copy link
Member

Trott commented May 15, 2018

Got better results this time, but still some spurious ones in arm-fanned. Trying that one again: https://ci.nodejs.org/job/node-test-commit-arm-fanned/1231/

@Trott
Copy link
Member

Trott commented May 15, 2018

I'm wondering if all my terminating of Pi jobs in process is what's causing the problem for arm-fanned. Going to let the last one finish this time. Again: https://ci.nodejs.org/job/node-test-commit-arm-fanned/1232/

@Trott
Copy link
Member

Trott commented May 15, 2018

(Other than arm-fanned, CI is totally green.)

@Trott
Copy link
Member

Trott commented May 15, 2018

arm-fanned is green too now. Landing...

@Trott
Copy link
Member

Trott commented May 15, 2018

Oops, not landing because it hasn't been approved for fast-tracking.

@vsemozhetbyt
Copy link
Contributor Author

Node.js Collaborators, please, add 👍 here if you approve fast-tracking.

@devsnek
Copy link
Member

devsnek commented May 15, 2018

the first commit only modifies two package-lock.json files?

@devsnek
Copy link
Member

devsnek commented May 15, 2018

@Trott is there not also a package.json to update?

@Trott
Copy link
Member

Trott commented May 15, 2018 •
edited
Loading

@Trott is there not also a package.json to update?

It's not necessary, but there could be one. That would require publishing a new version of remark-preset-lint-node to change ^1.0.0 to ^1.0.3 in one line of the relevant package.json. Didn't seem worth it since npm install will grab 1.0.3 anyway. But, if you want, I'll totally do it if there might be something I'm missing or even if just for aesthetic reasons.

@devsnek
Copy link
Member

devsnek commented May 16, 2018

@Trott if it isn't needed its fine, i was just confused when i saw lock files changed without the actual package changing is all

@vsemozhetbyt
Copy link
Contributor Author

@Trott Are we good to land?

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 18, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 18, 2018
@Trott @BridgeAR
tools: update prohibited-strings md linting
40469b4 
Update to remark-lint-prohibited-strings 1.0.3 to avoid false positives
with GitHub team names which must be lowercased no matter what our
prohibited-strings rules say about a particular string.

PR-URL: nodejs#20742
Refs: nodejs#20740
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 18, 2018
@vsemozhetbyt @BridgeAR
doc: fix typo in COLLABORATOR_GUIDE.md
59f71ea 
PR-URL: nodejs#20742
Refs: nodejs#20740
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR
Copy link
Member

Landed in 40469b4, 59f71ea 🎉

@BridgeAR BridgeAR closed this May 18, 2018
@vsemozhetbyt vsemozhetbyt deleted the doc-cc-typo branch May 18, 2018 15:31
@Trott
Copy link
Member

Trott commented May 19, 2018

@Trott Are we good to land?

@vsemozhetbyt A belated "yes". :-D

@Trott Trott mentioned this pull request May 19, 2018
Closed
4 tasks
MylesBorins pushed a commit that referenced this pull request May 22, 2018
@Trott @MylesBorins
tools: update prohibited-strings md linting
bd500af 
Update to remark-lint-prohibited-strings 1.0.3 to avoid false positives
with GitHub team names which must be lowercased no matter what our
prohibited-strings rules say about a particular string.

PR-URL: #20742
Refs: #20740
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 22, 2018
@vsemozhetbyt @MylesBorins
doc: fix typo in COLLABORATOR_GUIDE.md
9a8cdc9 
PR-URL: #20742
Refs: #20740
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax addaleax mentioned this pull request May 22, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@mhdawson mhdawson mhdawson approved these changes

@trivikr trivikr trivikr approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@vsemozhetbyt @BridgeAR @ofrobots @Trott @devsnek @jasnell @cjihrig @mhdawson @trivikr @nodejs-github-bot