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

Documentation for the Council Module #2281

Closed
wants to merge 47 commits into from

Conversation

ltfschoen
Copy link
Contributor

@ltfschoen ltfschoen commented Apr 15, 2019

Relates to polkadot-developers/substrate-developer-hub.github.io#27

Requires update for consistency with #2172

@ltfschoen ltfschoen added the A3-in_progress Pull request is in progress. No review needed at this stage. label Apr 15, 2019
@ltfschoen ltfschoen added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Apr 17, 2019
//! - **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
Copy link
Contributor

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.

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

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.

Copy link
Contributor Author

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

@kianenigma
Copy link
Contributor

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.

//!
//! #### Council Proposals
//!
//! - **Council** Defined in the [Substrate Developer Hub Glossary](https://docs.substrate.dev/docs/glossary#section-council).
Copy link
Member

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.

@joepetrowski joepetrowski self-requested a review April 24, 2019 19:23
Copy link
Contributor

@Demi-Marie Demi-Marie left a comment

Choose a reason for hiding this comment

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

Minor nits.

srml/council/src/lib.rs Outdated Show resolved Hide resolved
srml/council/src/lib.rs Show resolved Hide resolved
srml/council/src/lib.rs Outdated Show resolved Hide resolved
srml/council/src/lib.rs Outdated Show resolved Hide resolved
srml/council/src/lib.rs Outdated Show resolved Hide resolved
srml/council/src/lib.rs Outdated Show resolved Hide resolved
Co-Authored-By: ltfschoen <ltfschoen@users.noreply.github.com>
Copy link
Contributor

@Demi-Marie Demi-Marie left a 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


#[cfg(test)]
mod tests {
// These re-exports are here for a reason, edit with care
Copy link
Contributor

Choose a reason for hiding this comment

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

Spaces → tabs

Copy link
Contributor Author

@ltfschoen ltfschoen Apr 29, 2019

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.

Copy link
Contributor

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.

Copy link
Contributor

@Demi-Marie Demi-Marie left a comment

Choose a reason for hiding this comment

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

Looks good!

@ltfschoen
Copy link
Contributor Author

ltfschoen commented May 3, 2019

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

@ltfschoen ltfschoen closed this May 3, 2019
@kianenigma kianenigma deleted the luke-27-council-module-docs branch August 23, 2019 12:13
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants