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

Conversation

@joepetrowski
Copy link
Contributor

Supersedes #2281

I removed the list of functions in 572c1c4 because it is massive and I think it will be hard to remain. Open to revert if people want it in.

@joepetrowski
Copy link
Contributor Author

All three modules within this module provide a lot of functionality. I am considering the idea of having the lib.rs file just focus on the sub-module interaction and link to separate documentation for each sub-module (i.e. seats, motions, and voting would all have a //! section more typical of entire modules). What do others think?

Copy link
Contributor

@ltfschoen ltfschoen left a comment

Choose a reason for hiding this comment

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

ltgm aside from some nitpicks

@ltfschoen

This comment has been minimized.

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

This is indeed a more concise doc comparing to the old one. Some suggestions aside from the individual comments:

  • The concept of elevated a proposal was still unclear to me after reading the entire thing.
  • Maybe add a link to each heading so that it is clear which subsection of Terminology is related to which file (motion.rs etc.)
  • I couldn't find an inconsistency between anything in the doc and the code. What is covered, is correct but this does not mean that you've covered everything. If the goal is to make this doc a bit more in-depth, what I recommend is:
  • Similar to Staking doc add a usage scenario section in which you explain the main workflow of each of the three modules (seats, voting, motion). Best way to do so: Find that one most important test case in each module and explain it. In seats.rs this would be simple_tally_should_work() (from which you can see that the doc is missing concepts like tally, voting/presentation period, presenting, leaderboard and carry etc). A similar test case that demonstrates the main goal of the module (i.e. regardless of the details the main goal of seats.rs is to manage the 24 council seats.) should be identifiable and should lead you toward the next depth.

All that said, if the goal is is to keep the doc simple/minimal (at least for the first PR) I see this as a good starting point and has my implicit approval. The above recomendation is valid in case we want to take this a bit further in depth.

@joepetrowski
Copy link
Contributor Author

The concept of elevated a proposal was still unclear to me after reading the entire thing.

Hopefully fixed in next commit.

Maybe add a link to each heading so that it is clear which subsection of Terminology is related to which file

Done.

if the goal is is to keep the doc simple/minimal (at least for the first PR)

No, let's not merge any more docs that we think are incomplete.

Copy link

@DemiMarie-temp DemiMarie-temp left a comment

Choose a reason for hiding this comment

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

A couple nits

@kianenigma
Copy link
Contributor

kianenigma commented May 24, 2019

I have given the text a bit of more details and made sure it is correct, if not complete, up to my understanding and level of scrupulousness in limited time. LGTM. except for the code snippet that I really don't get + think it is not a good example. @joepetrowski I'd like to discuss this first and then change it with your consent.

@kianenigma kianenigma requested review from gavofyork and ltfschoen May 24, 2019 13:35
@joepetrowski joepetrowski added A0-please_review Pull request needs code review. M3-docs labels Jun 4, 2019
@joepetrowski joepetrowski removed the A0-please_review Pull request needs code review. label Jun 11, 2019
@ltfschoen
Copy link
Contributor

i've received feedback from rpmeier suggesting that we providing information about what origin the proposed call is executed with

@kianenigma
Copy link
Contributor

i've received feedback from rpmeier suggesting that we providing information about what origin the proposed call is executed with

In short: Democracy (public referendum) is root. Councillors can also execute with internal voting with Member<approvals>.

@ltfschoen
Copy link
Contributor

@rpmeier fyi

@joepetrowski joepetrowski changed the base branch from master to v1.0 June 30, 2019 18:44
@shawntabrizi shawntabrizi self-assigned this Jul 2, 2019
@shawntabrizi shawntabrizi requested a review from gavofyork July 2, 2019 17:07
@shawntabrizi shawntabrizi added A0-please_review Pull request needs code review. and removed A4-gotissues labels Jul 2, 2019
@gavofyork gavofyork merged commit 2f1b89f into v1.0 Jul 3, 2019
@gavofyork gavofyork deleted the council-docs branch July 3, 2019 12:44
@ltfschoen
Copy link
Contributor

@shawntabrizi why was the documentation commit history reset?

@ltfschoen
Copy link
Contributor

oh i see, it looks like it was removed based on the discussion comment that the changelog was too long. i thought squash and merge would have been sufficient.

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.

8 participants