-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Conversation
…ubstrate into luke-27-council-module-docs
srml/council/src/lib.rs
Outdated
//! - **Council proposal vote** A vote of yay or nay from a councillor on a single proposal that is stored in the | ||
//! `ProposalVoters`, `CouncilVoteOf`, and `Voting` mappings. Councillors may change their vote. | ||
//! - **Council proposal veto** A council member may veto any council proposal that is stored in `ProposalVoters` only | ||
//! once. A valid veto cancels a proposal, but a veto is not considered a vote. It is stored in `VetoedProposal` for a |
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.
A valid veto cancels a proposal, but a veto is not considered a vote
A bit ambiguous.
srml/council/src/lib.rs
Outdated
//! presentation period) to fill the seat if removal means that the desired members are not met. | ||
//! - `set_presentation_duration` - Set the presentation duration. | ||
//! - `set_term_duration` - Set the term duration. | ||
//! - `on_finalize` - Signature declaration that runs anything that needs to be done at the end of the block. |
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.
finalize_block
is not really council-related per se.
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.
thanks i've removed it. it says what finalize_block
is for in paritytech/substrate/core/sr-primitives/src/traits.rs anyway
I will add a minimal code example for the seats selection to this tomorrow. General overview of the doc: Being a complicated module I prefer the Council as well to be treated a slightly different and give it a bit more room to be verbose, explain the story and rational etc. The current doc does that in terms of going through the terminology. Yet, I see the big picture entirely missing (which is understandable given that the doc author is not the coder and the module is complicated). I think the least that can be done here is for the terminology bullet points to be sorted in some sensible way? Also, I don't know if having parts of the code comments as doc is ok or not. Maybe a good choice in favour of time but in the long term I think the doc should deliver more + easily understandable info, while the code comments are usually not like this. I would recommend enhancing the format of this doc and merge it as an initial version and to have consistency in the level of doc contributed to each module, yet create a dev-hub issue to enhance this. I don't see making it perfect content-wise to be a trivial task, so it might have to stay for after Sub0. |
…ubstrate into luke-27-council-module-docs
srml/council/src/lib.rs
Outdated
//! | ||
//! #### Council Proposals | ||
//! | ||
//! - **Council** Defined in the [Substrate Developer Hub Glossary](https://docs.substrate.dev/docs/glossary#section-council). |
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.
Let's please not link to outside resources. The reference docs should be the ultimate source of truth.
Co-Authored-By: ltfschoen <ltfschoen@users.noreply.github.com>
… considered a vote
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.
Minor nits.
Co-Authored-By: ltfschoen <ltfschoen@users.noreply.github.com>
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.
Parity uses a single tab for indentation
srml/council/src/lib.rs
Outdated
|
||
#[cfg(test)] | ||
mod tests { | ||
// These re-exports are here for a reason, edit with care |
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.
Spaces → tabs
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.
Thanks i've pushed a commit to restore the indentation now after reviewing the style guide https://wiki.parity.io/Substrate-Style-Guide. Is it ok now?
Note that I had to use character spacing in the Rustdoc section otherwise the formatting appeared incorrectly in 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.
There is a problem with reviewing indentation in GitHub. Not sure if it has to do with my browser dimensions or if other people have it too. I've had correctly-formatted blocks in VS Code and the rendered docs be correct, but when reviewing on GitHub, it's a complete mess. We always want the final html to be correct, so we just have to make sure to download the branch and run cargo doc
to see how it looks before changing.
…trate style guide
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.
Looks good!
Some of the implementation code (under this line of code https://github.com/paritytech/substrate/pull/2281/files#diff-4b33d936907774f9f47492facc3f3ac4L21) was accidently modified unintentionally when trying to merge the latest master and fixing indentation. Joe suggested it'd be better to just create a fresh PR #2447 where he's added some further changes in addition to fixing the accident that I caused |
Relates to polkadot-developers/substrate-developer-hub.github.io#27
Requires update for consistency with #2172