Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

add new documentation guidelines #14115

Merged
merged 21 commits into from
May 26, 2023

Conversation

kianenigma
Copy link
Contributor

@kianenigma kianenigma commented May 10, 2023

Adds new guidelines in the /docs folder, and adds a new team to review PRs (touching /frame and a few places in /primitives) based on the new guidelines. This is a small step in "stopping the bleeding" in terms of "having poorly documented source-code" before we embark on fixing further high-level documentation/tutorials.

closes paritytech/polkadot-sdk-docs#1
closes paritytech/polkadot-sdk-docs#2

Example pallet who's doc have been improved with the guidelines here:

@kianenigma kianenigma added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. labels May 10, 2023
docs/CODEOWNERS Outdated Show resolved Hide resolved
@kianenigma kianenigma changed the title add new documentation style-guides add new documentation guidelines May 10, 2023
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Nice!

@@ -0,0 +1,227 @@
# Substrate Documentation Guidelines

This document is only focused on documenting parts of substrate that relates to its external API. The list of such crates can be found in [CODEOWNERS](./CODEOWNERS). Search for the crates that are auto-assigned to a team called `docs-audit`.
Copy link
Member

Choose a reason for hiding this comment

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

We could have some docs labels similar to needs-audit like needs-docs and the CI puts them automatically.
Not sure how much that gets us, but would make it easier for people to see that they need to update it.

Copy link
Contributor

@sacha-l sacha-l May 11, 2023

Choose a reason for hiding this comment

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

I support this idea fwiw, yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would defer this decision to be made as a part of paritytech/labels#29 by @the-right-joyce and @juangirini. As far as I know, the agreed that using labels for things like audit was not the right choice as it is not a permanent label (eg 'needs audit' -> 'audited').

Copy link
Contributor

Choose a reason for hiding this comment

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

It depends on how you want to see it. needs-docs could very well be a permanent label, if it means that the PR is worth documenting instead of whether it was already documented or not then it is a permanent label. Similarly we could think of needs-audit.
In that context needs-docs might need a better naming, but I see it useful too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I find the label be a redundant side effect of the auto-review rule, isn't that the case? we already will assign this team to review automatically. I think this is semantically the same as the label.

Copy link
Contributor

Choose a reason for hiding this comment

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

True, good point. Though the auto-review rule will only ask the @paritytech/docs-audit team for a review. Should we add a rule to require at least one approval from them on top of the approvals we already need?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eventually, yes, but let's first see if the docs-audit team has enough bandwidth for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

we got rid off the audit labels, for the security audits we will use Github projects in future. The process still needs to be documented, but as a preparation I already have here the project set up: https://github.com/orgs/paritytech/projects/103
I'm syncing with Regina from SRLabs regarding that.

When it comes to the docs update, I'd also say this should be part of the review process. The same way we're changing the release process and want reviewers to add/adapt the changelog if needed, I think it should also be in their responsibility to think if the PR is important for docs as well and respectively add @paritytech/docs-audit as reviewers and the docs team are the ones who decide if the docs should be updated or not - does that make sense?

docs/DOCUMENTATION_GUIDELINES.md Outdated Show resolved Hide resolved
docs/DOCUMENTATION_GUIDELINES.md Outdated Show resolved Hide resolved
docs/DOCUMENTATION_GUIDELINES.md Outdated Show resolved Hide resolved
docs/DOCUMENTATION_GUIDELINES.md Outdated Show resolved Hide resolved
docs/DOCUMENTATION_GUIDELINES.md Outdated Show resolved Hide resolved
- In particular, mind the maximal line length of 100 (120 in exceptional circumstances).
- There is no commented code checked in unless necessary.
- Any panickers in the runtime have a proof or were removed.
- [ ] **Runtime Version:** You bumped the runtime version if there are breaking changes in the **runtime**.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this no longer needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not relevant anymore, contributors don't bump this. only the release team manages it. @coderobe can you confirm?


The first question is, what should you document? Use the following filter:

1. Within the set of crates above,
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this line, it's unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kinda disagree 🙈 I think it is good to remind people that this only applies to certain crates. Instead, I will make it a bit cleared. Let me know what you think.


1. Within the set of crates above,
2. Any `pub` item within them needs to be documented. If it is not `pub`, it won't show up in the rust-docs, and won't matter.
* Within `pub` items, sometimes they are only `pub` in order to be used by another internal crate, and you can foresee that this will not be used by anyone else other than you. These need **not** be documented thoroughly, and are left to your discretion to identify.
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
* Within `pub` items, sometimes they are only `pub` in order to be used by another internal crate, and you can foresee that this will not be used by anyone else other than you. These need **not** be documented thoroughly, and are left to your discretion to identify.
* There are some `pub` items that are only `pub` in order to be accessible for another internal crate. These should still be documented although you do not need to be as thorough as you are with the `pub` items that are for external developer use.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we ask to add a comment like "//no-doc" to establish that the author intentionally didn't add the docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hummusonrails I am a bit hesitant about your suggestions that seem paraphrasing to me. Do you have strong opinions that the original text is wrong or poorly worded, or are you adding optional suggestions? I accepted most, but might close some. Please let me know if you prefer otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kianenigma In this one, I attempted to make it more neutral and formal, whereas your version was more informal and personal in tone. I'm of the opinion that documentation should lean more into the former. That's why I made that suggestion. Hope that helps clarify!


As mentioned [here](https://web.mit.edu/rust-lang_v1.25/arch/amd64_ubuntu1404/share/doc/rust/html/book/first-edition/documentation.html#writing-documentation-comments) and [here](https://blog.guillaume-gomez.fr/articles/2020-03-12+Guide+on+how+to+write+documentation+for+a+Rust+crate), always start with a **single sentence** demonstrating what is being documented. If there is more to say, add it *after a newline*.

About [special sections](https://web.mit.edu/rust-lang_v1.25/arch/amd64_ubuntu1404/share/doc/rust/html/book/first-edition/documentation.html#special-sections), we will most likely not need to think about panic and safety in any runtime related code. Our code is never `unsafe`, and will (almost) never panic.
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
About [special sections](https://web.mit.edu/rust-lang_v1.25/arch/amd64_ubuntu1404/share/doc/rust/html/book/first-edition/documentation.html#special-sections), we will most likely not need to think about panic and safety in any runtime related code. Our code is never `unsafe`, and will (almost) never panic.
About [special sections](https://web.mit.edu/rust-lang_v1.25/arch/amd64_ubuntu1404/share/doc/rust/html/book/first-edition/documentation.html#special-sections), we will most likely not need to think about panic and safety in any runtime related code. Our code is never `unsafe`, and will (almost) never panic. If you see a panic in the code that is a reason to raise a question to the developer who introduced it rather than work to document it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can you elaborate more what you are suggesting? in general, you might see things like expect() in the code but that is acceptable. Did you take that into account?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was trying to make explicit that a code documenter should flag a panic and seek clarification before documenting as this in the "How to document" section. Very open to alternative ways to do so!

Copy link
Contributor

@juangirini juangirini left a comment

Choose a reason for hiding this comment

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

LGTM
Not sure whether you want to enforce the pr-custom-review rule for the @paritytech/docs-audit team or not though. In case we want to do it, it could be addressed in a different PR

Co-authored-by: Sebastian Kunert <skunert49@gmail.com>
Copy link
Contributor

@skunert skunert left a comment

Choose a reason for hiding this comment

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

Looks very good!

Copy link
Contributor

@hummusonrails hummusonrails left a comment

Choose a reason for hiding this comment

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

@kianenigma, this is amazing work! Thanks for being so responsive on the myriad of feedback comments on this. Really excited to see it merged!

Copy link
Contributor

@sacha-l sacha-l left a comment

Choose a reason for hiding this comment

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

Looking good to me 👍🏻

Copy link
Contributor

@sacha-l sacha-l left a comment

Choose a reason for hiding this comment

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

Looking good to me 👍🏻

@kianenigma
Copy link
Contributor Author

bot merge

Copy link
Contributor

@muharem muharem left a comment

Choose a reason for hiding this comment

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

super useful!
it would be nice to have a note on what we do with Readme files. I've seen them empty, or duplicating the docs of a crate, or probably outdated.

@muharem
Copy link
Contributor

muharem commented May 26, 2023

Also may be worth to decide if we gonna use the Parameters: heading for calls to list the arguments with some comments.

Optionally, in order to demonstrate the relation between the two, you can start the pallet documentation with:

```
//! > Made with *Substrate*, for *Polkadot*.
Copy link
Contributor

Choose a reason for hiding this comment

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

sounds a bit exclusive, when in reality used not only by Polkadot.

Copy link
Contributor

Choose a reason for hiding this comment

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

well to be fair "made for" doesn't mean it couldn't be used for other things, but this is a good point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a trial just for marketing to see. We are still yet to decide if we want to add this in all pallets and crates.

Ank4n pushed a commit that referenced this pull request Jul 8, 2023
* add new documentation style-guides

* update CODEOWNERS as well

* try new team

* revert

* Update docs/DOCUMENTATION_GUIDELINES.md

Co-authored-by: Juan <juangirini@gmail.com>

* Update docs/DOCUMENTATION_GUIDELINES.md

Co-authored-by: Juan <juangirini@gmail.com>

* Apply suggestions from code review

Co-authored-by: Ben Greenberg <13892277+hummusonrails@users.noreply.github.com>

* Apply suggestions from code review

Co-authored-by: Sam Johnson <sam@durosoft.com>
Co-authored-by: Ben Greenberg <13892277+hummusonrails@users.noreply.github.com>

* Update docs/DOCUMENTATION_GUIDELINES.md

* Apply suggestions from code review

Co-authored-by: Liam Aharon <liam.aharon@hotmail.com>
Co-authored-by: Ben Greenberg <13892277+hummusonrails@users.noreply.github.com>

* Apply suggestions from code review

Co-authored-by: Ben Greenberg <13892277+hummusonrails@users.noreply.github.com>
Co-authored-by: Liam Aharon <liam.aharon@hotmail.com>

* Apply suggestions from code review

Co-authored-by: Sacha Lansky <sacha@parity.io>
Co-authored-by: Liam Aharon <liam.aharon@hotmail.com>

* Further updates.

* note about rewrap.

* update ToC and more

* update ToC and more

* Update docs/DOCUMENTATION_GUIDELINES.md

Co-authored-by: Sebastian Kunert <skunert49@gmail.com>

* storage

---------

Co-authored-by: Juan <juangirini@gmail.com>
Co-authored-by: Ben Greenberg <13892277+hummusonrails@users.noreply.github.com>
Co-authored-by: Sam Johnson <sam@durosoft.com>
Co-authored-by: Liam Aharon <liam.aharon@hotmail.com>
Co-authored-by: Sacha Lansky <sacha@parity.io>
Co-authored-by: Sebastian Kunert <skunert49@gmail.com>
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* add new documentation style-guides

* update CODEOWNERS as well

* try new team

* revert

* Update docs/DOCUMENTATION_GUIDELINES.md

Co-authored-by: Juan <juangirini@gmail.com>

* Update docs/DOCUMENTATION_GUIDELINES.md

Co-authored-by: Juan <juangirini@gmail.com>

* Apply suggestions from code review

Co-authored-by: Ben Greenberg <13892277+hummusonrails@users.noreply.github.com>

* Apply suggestions from code review

Co-authored-by: Sam Johnson <sam@durosoft.com>
Co-authored-by: Ben Greenberg <13892277+hummusonrails@users.noreply.github.com>

* Update docs/DOCUMENTATION_GUIDELINES.md

* Apply suggestions from code review

Co-authored-by: Liam Aharon <liam.aharon@hotmail.com>
Co-authored-by: Ben Greenberg <13892277+hummusonrails@users.noreply.github.com>

* Apply suggestions from code review

Co-authored-by: Ben Greenberg <13892277+hummusonrails@users.noreply.github.com>
Co-authored-by: Liam Aharon <liam.aharon@hotmail.com>

* Apply suggestions from code review

Co-authored-by: Sacha Lansky <sacha@parity.io>
Co-authored-by: Liam Aharon <liam.aharon@hotmail.com>

* Further updates.

* note about rewrap.

* update ToC and more

* update ToC and more

* Update docs/DOCUMENTATION_GUIDELINES.md

Co-authored-by: Sebastian Kunert <skunert49@gmail.com>

* storage

---------

Co-authored-by: Juan <juangirini@gmail.com>
Co-authored-by: Ben Greenberg <13892277+hummusonrails@users.noreply.github.com>
Co-authored-by: Sam Johnson <sam@durosoft.com>
Co-authored-by: Liam Aharon <liam.aharon@hotmail.com>
Co-authored-by: Sacha Lansky <sacha@parity.io>
Co-authored-by: Sebastian Kunert <skunert49@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Identify API + team to review it Create documentation guidelines