-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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 styleguide #41025
doc: add styleguide #41025
Conversation
I'll update this tomorrow with a definition on how to fit in the Changelog section in Markdown rather than YAML. Wanted to get this up ASAP, though :) |
Improved line lengths, ran |
@nodejs/documentation |
@bnb, this PR is confusing to me since I was not part of this discussion — it seems like this is the result of some @nodejs/next-10 discussions that took place recently, but perhaps even more recently was another discussion had by myself, @GeoffreyBooth, and @Trott… We are currently using the Microsoft Docs styleguide — would anyone like to explain why the change? I am not necessarily opposed to a change, but this seems to be contrary to what we have been using so far… |
doc/styleguide.md
Outdated
[nodejs/remark-preset-lint-node]: https://github.com/nodejs/remark-preset-lint-node | ||
[remark-preset-lint-node]: https://www.npmjs.com/package/remark-preset-lint-node | ||
[sentence-case]: https://apastyle.apa.org/style-grammar-guidelines/capitalization/sentence-case | ||
[title-case]: https://apastyle.apa.org/style-grammar-guidelines/capitalization/title-case |
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 am +1 to using title case for titles.
I still haven't figured out why we have not been using this from the beginning other than wanting to do what MDN does, which doesn't seem properly justified.
doc/styleguide.md
Outdated
|
||
The following rules only apply to the documentation of APIs. | ||
|
||
### Title and description |
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.
### Title and description | |
### Title and Description |
APA title case would need to be applied to all of these titles.
How can we lint for this? I would like to do so.
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.
APA title case would need to be applied to all of these titles.
This is not the title of the page and per the text above, should be sentence case.
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.
You are correct in that this not being the title of the page; however, it is the title of a section of the page. We can call them “chapters” as @bnb proposes.
Other words for this could be “segments”, “sections”, etc…
This feels like a very big change, essentially a rewrite/reformatting of all of Node’s documentation. While making the docs more parseable for type definitions is a worthy goal, I’m not sure that the level of effort required here justifies that payoff. I’m also a big fan of the policy that we just follow the Microsoft Style Guide and that’s it, and we spare ourselves debates over style questions. Adopting our own styleguide would seem to invite such debates. Maybe this was already discussed within Next-10 and raised to the TSC, but I think if this is something that the project wants to pursue it should be debated and approved by the TSC. cc @Trott, @nodejs/tsc |
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.
We already have a documentation style guide. https://github.com/nodejs/node/blob/e601c0d678f815e00c385c5b5b3b9efd6bcaaf91/doc/guides/doc-style-guide.md
We should not have two duplicative style guides and we should certainly not have two contradictory style guides.
doc/styleguide.md
Outdated
|
||
There are a few style guidelines that aren't covered by the linter rules: | ||
|
||
* Use `sh` instead of `cmd` in code blocks (due to the syntax highlighter). |
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.
The text says this isn't covered by the linter rules, but it in fact is.
doc/styleguide.md
Outdated
|
||
* Use `sh` instead of `cmd` in code blocks (due to the syntax highlighter). | ||
* Keep line lengths between 80 and 100 characters if possible for readability | ||
purposes. |
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.
The linter (mostly) enforces 80 character wrapping so this too is enforced by the linter but the text above says it isn't.
doc/styleguide.md
Outdated
purposes. | ||
* No nesting lists more than 2 levels (due to the markdown renderer). | ||
* All `js` and `javascript` code blocks are linted with | ||
[standard-markdown](https://www.npmjs.com/package/standard-markdown). |
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 don't believe this is correct.
doc/styleguide.md
Outdated
* No nesting lists more than 2 levels (due to the markdown renderer). | ||
* All `js` and `javascript` code blocks are linted with | ||
[standard-markdown](https://www.npmjs.com/package/standard-markdown). | ||
* For unordered lists, use asterisks instead of dashes. |
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.
Covered by lint rules.
Not sure if it isn't sufficiently high-quality, but there is already JSON-formatted documentation and there are consumers out there that use it, so significant changes there may break them. Here is the current "all JSON docs in one page" document: https://nodejs.org/dist/latest-v17.x/docs/api/all.json And here is an example of the JSON docs for |
I could be wrong about the extent of this, but there seems to be a desire for more JSDoc (to assist with IDEs, for example) so it might be worth bringing that into the discussion too. @aduh95 |
The key elements from the electron style guide that are needed to provide a JSON format that can be more effectively parsed is the use of the section headers and how we format parameters. I agree that we don't need another doc, instead I'd advocate for us integrating those key elements into the existing style guide. For example none of the things related to language usage etc. should be modified. In terms of a few comments in the discussion so far (not sure if discussion would work better in #40953) but the PR seems to have spurred more discussion :)
Agreed, I don't think the Microsoft style guide touches on what is needed to make the docs parseble and we should not wander into that territory.
It is more of a tweak in many cases than re-write. The technical content should stay the same, the major changes would be how headings are used and formatting of parameters. @bnb can you link the sample of the before/after for the one you modified as I think it is a good example of how similar they feel
Agreed. I'd already opened #40953 and tagged for the TSC agenda. Unfortuantely we cancelled the meeting last week and so it was not discussed before the PR was opened
This is what the Typescript team uses today, however, it's hard to parse and requires manual fixing afterwards. In the Next-10 mini-summit we asked if changes introduced to adopt the electron style guide would be an issue for them and they were more than happy to accept that in order to end up with a better format in the long term. The plan would be to continue to build the docs/existing JSON in the same was as today. As headings/parameters format was updated that format might change but I don't think anything in our existing guides would prevent that from happening since our existing guide is quite loose on structure. From what I see it says only
I asked specifically about that in the Next-10 summit as well. The feedback was that generating documentation from JSDoc may limit contributors who can contribute. This came from one attendee who identified as mostly working on the docs. I can understand that as the flow for documentation changes today is easier than for changes to the code in terms of requirements for running all tests and even how easy it is to get a green CI. We could do both JSDoc and the existing documentation but I worry a bit about keeping those in sync. It is an interesting aspect but if we are going to continue having non-JSDoc generated documentation it may be a separate concern. |
I have found this reference in the style guide - https://github.com/nodejs/node/blob/master/tools/doc/README.md. This may be where more change would potentially be introduced in the proposal. |
I think it would be good to replace this PR with an update to the existing style guide - https://github.com/nodejs/node/blob/master/doc/guides/doc-style-guide.md with only the changes that would be needed to make the doc's more parse-able. We could start with what will make them parse-able with the electron parser and then see if those changes are a minimal set or if tweaks to the parser would reduce the required changes. |
I'll also re-open the periodic can of worms: |
We could have links to all of those items in a See Also section in the README for discoverability. |
I do agree that we are using doc/guides to hold things that it was probably not originally intended for because we don't have anywhere else from those to go. |
Move the style guide to doc/README.md so people might find it. The current location is easily overlooked, as is evidenced by nodejs#41025
Move the style guide to doc/README.md so people might find it. The current location is easily overlooked, as is evidenced by nodejs#41025
I'm trying to get
I think before trying to adopt
|
Move the style guide to doc/README.md so people might find it. The current location is easily overlooked, as is evidenced by nodejs#41025
A package.json is required by most node-based tooling; that seems like a reasonable requirement. |
doc/README.md
Outdated
|
||
## Language | ||
|
||
### Spelling, Punctuation, Naming, and Referencing Rules |
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.
### Spelling, Punctuation, Naming, and Referencing Rules | |
### Spelling, punctuation, naming, and referencing Rules |
doc/README.md
Outdated
|
||
<!-- lint disable prohibited-strings remark-lint--> | ||
## Additional Context and Rules |
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.
## Additional Context and Rules | |
## Additional context and rules |
doc/README.md
Outdated
* When documenting APIs, every function should have a usage example or | ||
link to an example that uses the function. | ||
|
||
## API References |
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.
## API References | |
## API references |
doc/README.md
Outdated
|
||
## Class: Serializer | ||
|
||
### Instance Methods |
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.
### Instance Methods | |
### Instance methods |
doc/README.md
Outdated
|
||
## Class: Deserializer | ||
|
||
### Instance Methods |
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.
### Instance Methods | |
### Instance methods |
doc/README.md
Outdated
|
||
See also API documentation structure overview in [doctools README][]. | ||
## See Also |
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.
## See Also | |
## See also |
The resulting document's organization seems like it could stand some improvement. It starts off with headings (exactly one On the one hand, non-blocking. On the other hand, we probably want the document about documentation to be an exemplar of how to write documentation. |
doc/README.md
Outdated
backslash-escaping: `\_`, `\*`, and ``\` ``. | ||
* When using underscores, asterisks, and backticks, please use | ||
backslash-escaping: `\_`, `\*`, and ``\` ``. | ||
* No nesting lists more than 2 levels (due to the markdown renderer). |
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.
What problem does the markdown renderer run into while displaying lists with > 2 levels? And what would you suggest as a workaround in places like
Lines 3630 to 3636 in 63503a4
* `options`: {Object} | |
* `length`: {number} The bit length of the key to generate. This must be a | |
value greater than 0. | |
* If `type` is `'hmac'`, the minimum is 1, and the maximum length is | |
2<sup>31</sup>-1. If the value is not a multiple of 8, the generated | |
key will be truncated to `Math.floor(length / 8)`. | |
* If `type` is `'aes'`, the length must be one of `128`, `192`, or `256`. |
doc/README.md
Outdated
* No nesting lists more than 2 levels (due to the markdown renderer). | ||
* For unordered lists, use asterisks instead of dashes. | ||
|
||
### Document Rules |
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.
rules
has >= 4 letters, so shouldn't it be capitalized? (I think that's what the How to implement title case section says)
In an effort to avoid perfection from being the enemy of good, since @Trott flagged it as non-blocking I'd suggest we land this and then aim to improve in follow on PRs |
doc/README.md
Outdated
* Chapters in the same page must have `##`-level headings. | ||
* Sub-chapters need to increase the number of `#` in the heading according to | ||
their nesting depth. | ||
* The page's title must follow [APA title case][title-case]. |
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 don't think we should cite APA here or anywhere else. It muddies the waters. We already have a style guide (Microsoft's) and adding others just makes things more confusing.
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.
* The page's title must follow [APA title case][title-case]. | |
* The page's title must be in title case. | |
* ``` |
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 was wondering if it makes sense to avoid mixing APA title case and Microsoft's sentence case and just stick to sentence case for all titles because that's what the Microsoft style guide suggests and most of the documentation already adheres to that. Is it a requirement of https://github.com/electron/docs-parser?
doc/README.md
Outdated
* Sub-chapters need to increase the number of `#` in the heading according to | ||
their nesting depth. | ||
* The page's title must follow [APA title case][title-case]. | ||
* All chapters must follow [sentence case][sentence-style]. |
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.
* All chapters must follow [sentence case][sentence-style]. | |
* All chapters must be in sentence case. |
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 don't like the use of "chapters". No one understands it at first. It seems that only after reading through the document, you figure out that "chapters" are "headings, but not the first one". Which is a confusing use of the term, in my opinion. Let's not use words in idiosyncratic ways if we can avoid it.
doc/README.md
Outdated
* The page's title must follow [APA title case][title-case]. | ||
* All chapters must follow [sentence case][sentence-style]. | ||
|
||
### Markdown Rules |
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.
### Markdown Rules | |
### Markdown rules |
The more I look at this, the more I think that correcting that issue will make reviewing easier, and that landing it without correcting that issue probably means somethings get lost in the shuffle. |
@bnb Are you OK if I add changes to this myself as fixup commits (so you can pull any out you don't like)? |
Ah yes my apologies, feel free @Trott :) |
Signed-off-by: Tierney Cyren <[email protected]>
6fa29f1
to
f5aee2a
Compare
@bnb I squashed your four commits into a single commit, and then added four individual fixup commits. Can you review the fixup commits and confirm that they are acceptable to you? If so, I'll move on to what I think would be the final round of consolidation etc. |
This has been dormant for long enough that it seems like we should either land it or close it. I suspect that trying to change the documentation format as described here is highly likely conflict with the documentation restructuring and tooling that @ovflowd is in the middle of doing right now. So I'm going to close this. But if anyone feels strongly that it should stay open and we should try to make this work, please re-open or leave a comment. |
This PR adds a documentation style guide. This comes as a result of #40953 and the various next-10 discussions that led to it. It is largely a copy/paste from electron/electron's styleguide.md, with some changes.
The goal of this is not to have it as an immediately enforceable, but rather to have it be planted as the guide that we'll eventually fully enforce on all documentation that it can be enforced on (some documentation, like native APIs, it can only be partially enforced on).
Landing this and, eventually, having all of our documentation written in this style should allows us the ability to use docs-parser which produces a high-quality JSON output of markdown documentation written as the style guide defines. This would allow the DefinitelyTyped people to produce higher quality type definitions, potentially even just using Electron's typescript-definitions tooling that parses docs-parser output into
.d.ts
files.There are other, less objective benefits:
Worth noting, I also created #32206 some time ago, and this is one step towards that as a reality. In doing so, I also created bnb/node-docs-parser which snapshotted a few examples from the Node.js documentation at the time I created it and provided an example of what those documents would look like if they were migrated. While it's been a pandemic worth of time since I last worked on that repo and some of the docs have been updated (querystring in particular is much more consistent!), the documents in
/docs/api
are still pretty close to what the "translated" versions would look like if you want a reference.changes to the styleguide.md from the e/e version include:
Electron
>Node.js
)markdownlint
a few things to discuss:
and just for context, the ideal next steps after this PR lands would be to begin incrementally migrating documentation to fit the style guide.