This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
add new documentation guidelines #14115
add new documentation guidelines #14115
Changes from 4 commits
2bfe4ba
29ded79
4392e1a
a02795b
0674e9c
184d5da
6ae9c3c
29ed2d0
160f32e
ac3f6e0
23b8595
073aae9
2f71f64
8855ad1
746f380
55d8872
401e171
121e251
a34d7ae
9ac12e9
0211eac
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 could have some docs labels similar to
needs-audit
likeneeds-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.
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 support this idea fwiw, yes
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 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').
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.
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 ofneeds-audit
.In that context
needs-docs
might need a better naming, but I see it useful too.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 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.
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.
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?
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.
Eventually, yes, but let's first see if the docs-audit team has enough bandwidth for this.
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 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?
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.
Remove this line, it's unnecessary.
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 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.
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.
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.
Should we ask to add a comment like "//no-doc" to establish that the author intentionally didn't add the docs?
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.
@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.
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.
@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!
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.
One common convention is to expose such items via a
#[doc(hidden)]
public module called__private
from the crate root. In fact this is used in at least 5 popular crates I can think of, including the entire syn/quote ecosystem. Maybe worth suggesting a convention here?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.
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.
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?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 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!
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.
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.
Do you expect authors to write 1-2 sentences addressing this? I saw in the
fast-unstake
pallet you just left it at:See pallet module for more information.
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.
good point, let me re-asses.
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.
Updated. I think in 99% of the time this section can be copy-pasted. We could even consider auto-generating it, but given that we want it to come after
## Overview
, it might be hard to do.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.
Why is this no longer needed?
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.
This is not relevant anymore, contributors don't bump this. only the release team manages it. @coderobe can you confirm?