-
Notifications
You must be signed in to change notification settings - Fork 30k
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
doc: add new documentation lint rule #18726
Conversation
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.
In general this is LGTM.
and | ||
`https://github.com/search?q=commenter%3A${GITHUB_ID}++org%3Anodejs&type=Issues` | ||
`https://github.com/search?q=commenter%3A${GITHUB_ID}++org%3Anodejs&type=Issues` |
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.
Hm, these two changes look somewhat weird but I can imagine that the new rule will otherwise complain about it even though it should not?
It would be great to ignore the changelog file but I guess it is not that bad if we don't. What do others think? Besides that I am a bis surprised that the md files in docs are not linted. They are files that contain lines above 80 characters e.g. @watilde is it possible to ignore files somehow? |
@estrada9166 seems like the linter will now properly complain about the rest of the entries above 80 characters. Can you please also fix those? See https://ci.nodejs.org/job/node-test-linter/16080/console. Those errors should also come up when executing |
@BridgeAR Sure thing! fixing right now! |
@estrada9166 - thanks for stepping up and making a PR :) @nodejs/collaborators - is everyone OK with an 80 character limit in our docs? Personally I'm -0 on it. Making sure there are no strong objections since this is a style rule. |
In theory I'm in favour of it but I would like some clarity re: the long URLs. Is the change highlighted by @BridgeAR above necessary? If so, that makes me look less favourably on introducing this rule without first fixing it upstream to be a bit smarter. |
@benjamingr we already try to uphold that limit in the docs. Just manually and that fails some times. Therefore the request to add the rule. |
@apapirovski long URLs should definitely not cause issues. In case we have issues with those, I would suggest we fix it upstream and then get this in. @watilde do you know if there is an option for things like that? Otherwise, would you be so kind and implement such an option or give a short hint where to work on to get it fixed upstream? |
@BridgeAR I just updated |
@estrada9166 as I pointed out, it would be good if we find a way to exclude those. So thanks a lot for the work you already did! |
I'm -1 to including changelogs for this. |
I'll contine searching how we can ignore them, because when I add them to |
@wooorm would you be so kind and take a look / give advice? |
It doesn't seem like remark supports stacking multiple |
Ignoring: you can either go with globs to files (
Well, config files can be JS, and can link to other configs, so you can do: // .remarkrc
{
"plugins": [
"remark-lint-something",
"./some-other-file"
]
} ...and // ./some-other-file.js
exports.plugins = [require('remark-lint-something-else')] |
P.S. feel free to ask specific questions, I’d love to help, but I am also a bit swamped with a course (on node 🎉) I’m currently giving! |
Thanks @wooorm. Due to how everything is structured within Node.js repo, it seems the file within
Specifying simply |
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.
I'm -1 to this change (I'm making this explicit with the big red cross).
I see too many changes in our changelog, which I would prefer if this didn't touch.
I think this change could impact backporting doc changes too
Can we pick up a number that reflects the current situation (or causes less changes)?
We could pick the limit based on the max line length we have in the changelog.
@mcollina read the above discussion ;-) |
@BridgeAR My solution to exclude the
Excluding them on |
I think that is a pretty elegant and easy solution! In this case it is great because it is much faster than having to upstream a change first. Just one more thing: I think it would also be good to exclude the |
@BridgeAR Done! |
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
Add 80 characters limit to docs. Change docs to fit 80 characters per row. PR-URL: nodejs#18726 Fixes: nodejs#18703 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Landed in a29089d @estrada9166 congratulations on your first commit to Node.js 🎉 |
Thank you so much to all of you!! 🎉 |
Should this be backported to |
Add 80 characters limit to docs. Change docs to fit 80 characters per row. Backport-PR-URL: #19189 PR-URL: #18726 Fixes: #18703 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Add 80 characters limit to docs. Change docs to fit 80 characters per row. PR-URL: nodejs#18726 Fixes: nodejs#18703 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Add 80 characters limit to docs. Change docs to fit 80 characters per row. Backport-PR-URL: #19189 PR-URL: #18726 Fixes: #18703 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Add 80 characters limit to docs. Change docs to fit 80 characters per row. PR-URL: nodejs#18726 Fixes: nodejs#18703 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Add 80 characters limit to docs. Change docs to fit 80 characters per row. PR-URL: nodejs#18726 Fixes: nodejs#18703 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Should this be backported to |
Add 80 characters limit to docs.
Change docs to fit 80 characters per row.
Fix broken links in Changelog.md
Fixes: #18703
Refs: remarkjs/remark-lint#82
Also I had to change manually the documentation for tools/icu
the documentation is not clear to ignore files and with
.remarkignore
it shows an error
The docs must be changed in order to follow the commit guidelines.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
Doc, Tools