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

tools,doc: lint for additional strings in docs #17492

Closed
wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Dec 6, 2017

Check for uses of Github (rather than GitHub) and Node.JS (rather
than Node.js) in markdown files.

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

tools, doc

@Trott Trott added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Dec 6, 2017
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Dec 6, 2017
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM, but additional is spelled wrong in the commit message.

@Trott Trott changed the title tools,doc: lint for additinoal strings in docs tools,doc: lint for additional strings in docs Dec 6, 2017
Use `GitHub` rather than `Github` and `Node.js` rather than `Node.JS`.
Check for uses of `Github` (rather than `GitHub`) and `Node.JS` (rather
than `Node.js`) in markdown files.
@Trott
Copy link
Member Author

Trott commented Dec 6, 2017

Fixed commit message (and PR title) typo. (Thanks, @cjihrig!) Rebased against master while I was at it.

@@ -42,8 +42,10 @@ module.exports.plugins = [
[
require('remark-lint-prohibited-strings'),
[
{ no: 'v8', yes: 'V8' },
{ no: 'Javascript', yes: 'JavaScript' }
{ no: 'Github', yes: 'GitHub' },
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't tools/remark-preset-lint-node/index.js technically provided by watilde/remark-preset-lint-node, and shouldn't be edited downstream? Ref #17441

Copy link
Member Author

@Trott Trott Dec 6, 2017

Choose a reason for hiding this comment

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

It's a small thing, but I'd prefer that the version downstream get synchronized with this version (after this lands) rather than landing downstream first. If it lands downstream first, there's a race condition where new errors can creep in here before the updated version lands here. Not too big a deal for us as we can fix the problematic text, but potentially irksome for anyone installing the preset locally and suddenly getting errors on the Node.js master branch.

The problem with the approach I suggest above is that some info in the package.json will be wrong for a period of time. We're not running the version specified in the package.json. We're running a modified version and we need to update it again when this gets published upstream.

Anyway, I don't feel too strongly about this and can be persuaded that this should wait for downstream rather than the other way around if people feel strongly about it. Related: We should probably move the repo to the nodejs org and allow a few more folks to publish it, assuming @watilde has no problem with that.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a small thing, but I'd prefer that the version downstream get synchronized with this version (after this lands) rather than landing downstream first.

Ok, fine by me.

My other point (and why I put a link to my PR) was that this (very small) module should live within the nodejs/node repo, so that we don't have to worry about upstreaming/downstreaming changes and publishing versions to npm.

Copy link
Contributor

@maclover7 maclover7 left a comment

Choose a reason for hiding this comment

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

LGTM besides for question about remark-preset-lint-node

@Trott
Copy link
Member Author

Trott commented Dec 6, 2017

@Trott
Copy link
Member Author

Trott commented Dec 7, 2017

Raspberry Pi failures are build issues and unrelated. CI is good.

@Trott Trott added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Dec 7, 2017
Trott added a commit to Trott/io.js that referenced this pull request Dec 8, 2017
Use `GitHub` rather than `Github` and `Node.js` rather than `Node.JS`.

PR-URL: nodejs#17492
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Trott added a commit to Trott/io.js that referenced this pull request Dec 8, 2017
Check for uses of `Github` (rather than `GitHub`) and `Node.JS` (rather
than `Node.js`) in markdown files.

PR-URL: nodejs#17492
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@Trott
Copy link
Member Author

Trott commented Dec 8, 2017

Landed in 3ad01e7 and 762ed33.

@Trott Trott closed this Dec 8, 2017
@Trott
Copy link
Member Author

Trott commented Dec 8, 2017

Here's a PR to downstream to sync: https://github.com/watilde/remark-preset-lint-node/pull/3

MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
Use `GitHub` rather than `Github` and `Node.js` rather than `Node.JS`.

PR-URL: #17492
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
Check for uses of `Github` (rather than `GitHub`) and `Node.JS` (rather
than `Node.js`) in markdown files.

PR-URL: #17492
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
Use `GitHub` rather than `Github` and `Node.js` rather than `Node.JS`.

PR-URL: #17492
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
Check for uses of `Github` (rather than `GitHub`) and `Node.JS` (rather
than `Node.js`) in markdown files.

PR-URL: #17492
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
@gibfahn gibfahn added baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x and removed baking-for-lts PRs that need to wait before landing in a LTS release. labels Dec 20, 2017
@Trott Trott deleted the moar-strings branch January 13, 2022 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants