Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 26
feat: lint links and definitions#188
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
aduh95 commented Jun 27, 2021 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Replacement for checkLinks.mjs from the nodejs/node repo.
Trott left a comment
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.
LGTM once the necessary changes land in core
nschonni commented Jun 27, 2021 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
This rule seems like it's less nodejs specific than some of the others, so it might be worth having it as a standalone that is just used here |
aduh95 commented Jun 27, 2021
If someone wants to publish it standalone on npm, nothing's stoping them, but I don't feel like doing it myself (at least, not for the time being). |
PR-URL: nodejs#39170 Refs: nodejs/remark-preset-lint-node#188 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Darshan Sen <[email protected]>
Refs: nodejs/remark-preset-lint-node#188 PR-URL: #39165 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Harshitha K P <[email protected]>
aduh95 commented Jun 29, 2021
Tests are now passing, I think this is now ready to land. |
Trott commented Jun 30, 2021
I believe we usually treat new rules as semver-minor and save semver-major for things like dropping support of an older version of Node.js. |
PR-URL: #39170 Refs: nodejs/remark-preset-lint-node#188 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Darshan Sen <[email protected]>
Refs: nodejs/remark-preset-lint-node#188 PR-URL: #39165 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Harshitha K P <[email protected]>
SEWeiTung commented Jul 14, 2021
@Trott & @aduh95: |
Trott commented Jul 14, 2021 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
I wonder if there's a way to disable this check for localized/translated files. It seems like it would create extra surprising work unnecessarily for translators. Or at least maybe as a partial measure, ignore links that start with (or contain) non-ASCII characters? That would at least address the problem for languages where the ordering will surely change (like Arabic). What do you think, @aduh95? @MaledongGit It does a few things, but the one that is probably causing all the issues for translations is that it checks to make sure that the links listed at the bottom of the If we can't figure out something to make it work for translations, I'd consider removing the ordering part of the rule entirely. I also wonder if we should add nodejs.org to our CI test for this repo so we catch these things earlier. Translations are likely to run into all sorts of issues that we probably won't see in the main repo. |
aduh95 commented Jul 14, 2021
I suppose the solution could be to add a disable comment to the translated files: <!--lint disable remark-lint:nodejs-links--> |
SEWeiTung commented Jul 15, 2021 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
SEWeiTung commented Jul 15, 2021 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
BTW: Is it possible to disable in the ".remarkrc"? I suspect whether it's not a published package, so this cannot be used here...? |
nschonni commented Jul 15, 2021
@MaledongGit we already do https://github.com/nodejs/nodejs.org/blob/master/.remarkrc I think one thing might be to leave the English "references" at the bottom, as this is accepted |
nschonni commented Jul 15, 2021
It seems like there is a small case sensitivity issue with the current imlementation though. EX: from nodejs.org Why should the capitalized "CLI" be before "async" |
Trott commented Jul 15, 2021 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
This was for localization, ironically. The idea was that instead of alphabetical order, it should be in code-point order, and capital letters all come before lowercase letters that way. The thinking was that "alphabetical order" might not make any sense and/or be hard to code for in some languages (like Chinese), whereas ordering by code-point value is straightforward. Of course, what we're discovering now is that maybe the thing to do wasn't to try to accommodate these languages but rather to exempt them from the check altogether. |
Trott commented Jul 15, 2021
The following syntax is working for me: |
According to #3956, it seems we're ordering the links' titles with ASCII orders, however NOT all the docs (such as translations) and test\scripts\*.md, we DON'T need to cope with this rule, so we should disable it manually as a special case. Ref: 1. nodejs/remark-preset-lint-node#188. 2. nodejs/node#39170.
Refs: nodejs/remark-preset-lint-node#188 PR-URL: #39165 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Harshitha K P <[email protected]>
PR-URL: #39170 Refs: nodejs/remark-preset-lint-node#188 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Darshan Sen <[email protected]>





Replacement for checkLinks.mjs from the nodejs/node repo. Adds a validation for self-reference links to not contain the name of the file.