add new documentation guidelines

[kianenigma]

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:

• Improve documentation for fast-unstake pallet #14101

[ggwpez]

Nice!

[ggwpez]

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.

[sacha-l]

I support this idea fwiw, yes

[kianenigma]

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').

[juangirini]

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.

[kianenigma]

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.

[juangirini]

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?

[kianenigma]

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

[the-right-joyce]

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?

[juangirini]

Why is this no longer needed?

[kianenigma]

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

[hummusonrails]

Remove this line, it's unnecessary.

[kianenigma]

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.

[hummusonrails]

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.

[gupnik]

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

[kianenigma]

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

[hummusonrails]

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

[hummusonrails]

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.

[kianenigma]

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?

[hummusonrails]

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!

[juangirini]

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

[skunert]

Looks very good!

[hummusonrails]

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

[sacha-l]

Looking good to me 👍🏻

[sacha-l]

Looking good to me 👍🏻

[kianenigma]

bot merge

[muharem]

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]

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

[muharem]

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

[sam0x17]

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

[kianenigma]

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.