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

doc: add styleguide #41025

Closed
wants to merge 5 commits into from
Closed

doc: add styleguide #41025

wants to merge 5 commits into from

Conversation

bnb
Copy link
Contributor

@bnb bnb commented Nov 29, 2021

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:

  • highly consistent authoring experience that provides a consistent framework for writing and reviewing documentation
  • more digestible documentation for end-users, structuring all documentation in a hierarchy that's consistent across pages while still being specific enough to provide meaningful context regardless of content

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:

  • updating the name (Electron > Node.js)
  • removing references to markdownlint
    • given that Electron and docs-parser use markdownlint, it might make sense to migrate to it.
  • removed a TODO above the linter rules, noting that they should be checked against what the electron/electron markdown parser checks for.
    • potentially worth investigating finding a middle ground and both using one set of tooling.
  • tweaked examples to fit Node.js. As we build out the documentation to fit this style guide, we should update these examples to me more thorough.
  • removed one example under the "function signature" heading on line 169, as I'm not immediately familiar of a class that has the same name as its module and can be used as an example. Happy to re-add this when I find a good example.
    • here's the original text, for reference:
      • For example, the methods of the Session class under the session module must use ses as the objectName.

  • removed a reference on line 181 to Electron's cookie API custom sturcutre. Happy to re-add this back in once we've got our own custom structures added.
  • added backticks to line 228 where they weren't in the Electron documentation. I believe the backticks are allowed/required, but should double check just to make sure that's a correct assertion before merging.

a few things to discuss:

  • js code linting
    • Electron's standardized on StandardJS and has proper tooling for it, including standard-markdown. While I'm personally fine with this, I assume the Node.js project would want a different linter solution documented.
    • happy to change the text on lines 55-56 referencing this if we don't want to keep this.
  • I'm not sure if we have platform-specific functionality. If we don't and never plan on doing so, happy to remove line 194-203.
  • how we want to approach inline YAML changelogs. Particularly, it was raised that it would be nice to have in Markdown but it may also become burdensome for those who backport, which is definitely not something I'd want to see.
    • potential solution here is to document an additional markdown structure for changelogs and then have a small tool convert YAML into that Markdown structure as a build step.
    • I'm unopinionated on what a solution is here, but ideally, we will eventually not have any YAML frontmatter.

and just for context, the ideal next steps after this PR lands would be to begin incrementally migrating documentation to fit the style guide.

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Nov 29, 2021
@bnb
Copy link
Contributor Author

bnb commented Nov 29, 2021

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 :)

@bnb
Copy link
Contributor Author

bnb commented Nov 29, 2021

Improved line lengths, ran make format-md, and tossed in some backticks to examples so the linter doesn't screech at links that don't exist.

@targos
Copy link
Member

targos commented Nov 30, 2021

@nodejs/documentation

@DerekNonGeneric
Copy link
Contributor

@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…

[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
Copy link
Contributor

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 Show resolved Hide resolved

The following rules only apply to the documentation of APIs.

### Title and description
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
### 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.

Copy link
Member

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.

Copy link
Contributor

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…

@GeoffreyBooth
Copy link
Member

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

Trott
Trott previously requested changes Dec 2, 2021
Copy link
Member

@Trott Trott left a 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 Show resolved Hide resolved

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).
Copy link
Member

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.


* 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.
Copy link
Member

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.

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).
Copy link
Member

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.

* 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.
Copy link
Member

Choose a reason for hiding this comment

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

Covered by lint rules.

@Trott
Copy link
Member

Trott commented Dec 2, 2021

which produces a high-quality JSON output of markdown documentation

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 assert: https://nodejs.org/dist/latest-v17.x/docs/api/assert.json

@Trott
Copy link
Member

Trott commented Dec 2, 2021

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

@mhdawson
Copy link
Member

mhdawson commented Dec 2, 2021

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 :)

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.

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.

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.

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

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/i18n-api

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

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.

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 Documents must start with a level-one heading and does not say anything else about headings.

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

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.

@mhdawson
Copy link
Member

mhdawson commented Dec 2, 2021

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.

doc/styleguide.md Outdated Show resolved Hide resolved
@mhdawson
Copy link
Member

mhdawson commented Dec 3, 2021

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.

@Trott
Copy link
Member

Trott commented Dec 5, 2021

I'll also re-open the periodic can of worms: doc/guides makes people think those are guides for using Node.js and people often don't bother looking in there at all for stuff like a documentation style guide. I'm OK moving EVERYTHING out of that directory to more intuitive locations.

@Trott
Copy link
Member

Trott commented Dec 5, 2021

I'll also re-open the periodic can of worms: doc/guides makes people think those are guides for using Node.js and people often don't bother looking in there at all for stuff like a documentation style guide. I'm OK moving EVERYTHING out of that directory to more intuitive locations.

We could have links to all of those items in a See Also section in the README for discoverability.

@mhdawson
Copy link
Member

mhdawson commented Dec 7, 2021

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.

Trott added a commit to Trott/io.js that referenced this pull request Dec 9, 2021
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
Trott added a commit to Trott/io.js that referenced this pull request Dec 9, 2021
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
@Trott
Copy link
Member

Trott commented Dec 9, 2021

I'm trying to get docs-parser to work minimally with our docs.

  • It requires a package.json file in the project root. I added one.
  • It expects that docs reside in docs/api. (Ours are in doc/api.) I moved them.
  • It requires a structures subdirectory from there. I added it.
  • It complained about a parameter being shown as optional in a function signature but not being marked as optional in the parameter list in async_context.md. OK, sure. I marked it as optional.
  • Next, it fails while (mistakenly, I imagine) trying to interpret all our YAML metadata in buffer.md. I removed all the metadata. All 106 YAML blocks.
  • Then it complained about a parameter in buffer.md being shown as optional in a function signature but not being marked optional in the parameter list. Just like in async_context.md. We don't do this in any of our docs (because it was considered unnecessary--the optionality of the parameter is shown in the function signature) so OK, that's a style change. So...
  • I went to find the corresponding rule in the style guide added here to add it to our existing style guide, and it turns out that it goes unmentioned in the electron style guide that is the model for the style guide added here. It's shown in the examples and therefore I guess implied, but not stated.

I think before trying to adopt docs-parser, we may want to first submit the following changes:

  • Add a CLI flag or other option to not require a package.json. (I'm not sure why it's required though, so there may be complications there.)
  • Add a CLI flag or other option to specify a directory other than docs/api.
  • Find out what the issue is with it fighting with the YAML in our buffer.md file and fix either buffer.md or docs-parser so that they play nicely together.
  • Only then, update the style guide to accommodate doc-parser. (We could add a requirement to mark the optional parameters as optional in the parameter list. Maybe we could add a lint rule to remark-preset-lint-node and propose it for the style guide some time after doc: move style guide to findable location #41119 lands.)

Trott added a commit to Trott/io.js that referenced this pull request Dec 9, 2021
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
@ljharb
Copy link
Member

ljharb commented Dec 9, 2021

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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
### 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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
## 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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
## API References
## API references

doc/README.md Outdated

## Class: Serializer

### Instance Methods
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
### Instance Methods
### Instance methods

doc/README.md Outdated

## Class: Deserializer

### Instance Methods
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
### Instance Methods
### Instance methods

doc/README.md Outdated

See also API documentation structure overview in [doctools README][].
## See Also
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
## See Also
## See also

@Trott
Copy link
Member

Trott commented Apr 28, 2022

The resulting document's organization seems like it could stand some improvement. It starts off with headings (exactly one #, etc.), moves to markdown (use reference links rather than inline links, etc.), and then has a section confusingly named Document rules which contains information that I would expect to appear first (specifically, how documentation files are named).

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).
Copy link
Contributor

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

node/doc/api/crypto.md

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`.
? It renders okay here, right?

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
Copy link
Contributor

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)

@mhdawson
Copy link
Member

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.

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].
Copy link
Member

@Trott Trott Apr 28, 2022

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.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* The page's title must follow [APA title case][title-case].
* The page's title must be in title case.
* ```

Copy link
Contributor

@RaisinTen RaisinTen Apr 29, 2022

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].
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* All chapters must follow [sentence case][sentence-style].
* All chapters must be in sentence case.

Copy link
Member

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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
### Markdown Rules
### Markdown rules

@Trott
Copy link
Member

Trott commented Apr 28, 2022

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

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.

@mhdawson
Copy link
Member

@Trott ok if you feel it is a true blocker then good to know that. @bnb guess it needs some additional updates.

@Trott
Copy link
Member

Trott commented Apr 30, 2022

@bnb Are you OK if I add changes to this myself as fixup commits (so you can pull any out you don't like)?

@mhdawson
Copy link
Member

@bnb can you confirm with @Trott if you are ok with him adding fixup commits so that we can move this forward?

@bnb
Copy link
Contributor Author

bnb commented Jun 22, 2022

Ah yes my apologies, feel free @Trott :)

bnb and others added 2 commits June 24, 2022 06:10
Signed-off-by: Tierney Cyren <hello@bnb.im>
@Trott
Copy link
Member

Trott commented Jun 24, 2022

@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.

AugustinMauroy

This comment was marked as off-topic.

@Trott
Copy link
Member

Trott commented Apr 21, 2023

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.

@Trott Trott closed this Apr 21, 2023
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet