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

Documentation for balances module #1943

Merged
merged 35 commits into from
Mar 26, 2019
Merged

Documentation for balances module #1943

merged 35 commits into from
Mar 26, 2019

Conversation

joepetrowski
Copy link
Contributor

@joepetrowski joepetrowski commented Mar 7, 2019

First attempt at documentation for the balances module. Besides a general review, this needs:

  1. Particular attention to the "Usage" section of the readme. If you have corrections or better examples, please share.
  2. Continued discussion on the scope of rustdocs vs higher-level readme. I tried to account for the discussion emerging from Documentation for the timestamp module #1927.

TODO: Add documentation for tests. There are a lot of them and I'd like better direction on the scope of rustdocs before completing them, so wanted to post for review at this stage.

@parity-cla-bot

This comment has been minimized.

1 similar comment
@parity-cla-bot

This comment has been minimized.

@ltfschoen
Copy link
Contributor

ltfschoen commented Mar 7, 2019

Suggest reviewing latest comments in #1927.
And checking consistency in naming and usage of headings with polkadot-developers/substrate-developer-hub.github.io#44 (comment)

@joepetrowski
Copy link
Contributor Author

Suggest reviewing latest comments in #1927.
And checking consistency in naming and usage of headings with paritytech/substrate-developer-hub#44 (comment)

@ltfschoen based on our documentation chat yesterday, we are not following that template in paritytech/substrate-developer-hub#44 (comment) because we don't expect someone who modifies the code to modify the readme. Therefore, specific function behavior (including interfaces) goes in the rustdocs and the README.adoc file is for higher-level information and usage examples.

srml/balances/README.adoc Outdated Show resolved Hide resolved
srml/balances/README.adoc Outdated Show resolved Hide resolved
srml/balances/src/lib.rs Outdated Show resolved Hide resolved
srml/balances/src/lib.rs Outdated Show resolved Hide resolved
srml/balances/src/lib.rs Outdated Show resolved Hide resolved
srml/balances/src/lib.rs Outdated Show resolved Hide resolved
srml/support/src/traits.rs Outdated Show resolved Hide resolved
srml/balances/src/lib.rs Outdated Show resolved Hide resolved
srml/balances/README.adoc Outdated Show resolved Hide resolved
srml/balances/README.adoc Outdated Show resolved Hide resolved
srml/balances/src/lib.rs Outdated Show resolved Hide resolved
srml/balances/README.adoc Outdated Show resolved Hide resolved
srml/balances/src/lib.rs Outdated Show resolved Hide resolved
srml/balances/src/lib.rs Outdated Show resolved Hide resolved
@joepetrowski
Copy link
Contributor Author

Latest commit for conflict resolution after #2048

@joepetrowski joepetrowski dismissed shawntabrizi’s stale review March 25, 2019 07:37

lots of changes since review

srml/balances/src/lib.rs Outdated Show resolved Hide resolved
srml/balances/src/lib.rs Outdated Show resolved Hide resolved
srml/balances/src/lib.rs Outdated Show resolved Hide resolved
srml/balances/src/lib.rs Outdated Show resolved Hide resolved
srml/balances/src/lib.rs Outdated Show resolved Hide resolved
//!
//! The `Call` enum is documented [here](https://crates.parity.io/srml_balances/enum.Call.html).
//!
//! - `transfer` - Transfer some liquid free balance to another account.
Copy link
Contributor

Choose a reason for hiding this comment

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

I appreciate the unified quite full explaining top documentation but at the same time I feel like it may result in outdated documentation or unnecessary duplication, here I wonder if it not better to change to the same kind of redirection as module below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's why I kept the definitions really short here, so the detail is in the function documentation. I don't have a strong opinion about including them here or not, which is why I put the link to the Call documentation before the list.

gui1117 and others added 4 commits March 25, 2019 11:14
Co-Authored-By: joepetrowski <25483142+joepetrowski@users.noreply.github.com>
Co-Authored-By: joepetrowski <25483142+joepetrowski@users.noreply.github.com>
Co-Authored-By: joepetrowski <25483142+joepetrowski@users.noreply.github.com>
Co-Authored-By: joepetrowski <25483142+joepetrowski@users.noreply.github.com>
//! }
//! }
//! }
//! ```
Copy link
Contributor

Choose a reason for hiding this comment

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

I kind of agree for this example but at the same time it is not specific to balance, so I'm mixed but ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, not highly specific to balances. I thought our idea was to have an example with everything necessary to compile and use the module. Can remove if we think the real-use example is adequate.

srml/balances/src/lib.rs Outdated Show resolved Hide resolved
srml/balances/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@gavofyork gavofyork left a comment

Choose a reason for hiding this comment

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

I'll approve since there's nothing wrong with committing as-is, but you might want to bring back the contracts example.

@gavofyork gavofyork merged commit 72409f9 into master Mar 26, 2019
@gavofyork gavofyork deleted the joe-balances-docs branch March 26, 2019 16:00
//! pub struct Module<T: Trait> for enum Call where origin: T::Origin {
//! fn transfer_proxy(origin, to: T::AccountId, value: T::Balance) -> Result {
//! let sender = ensure_signed(origin)?;
//! <balances::Module<T>>::make_transfer(&sender, &to, value)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the updated function name is transfer() instead of make_transfer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, and more generally throughout some of the early docs whose modules had a refactor. #2159

MTDK1 pushed a commit to bdevux/substrate that referenced this pull request Apr 12, 2019
* comment updates

* added rustdoc and readme

* clarified LockableCurrency trait

* Currency trait rustdocs

* fixed typo

* fixed suggestions round 1

* UpdateBalanceOutcome docs (open for discussion)

* rm description of enum, consolidation, rm ReclaimRebate

* type clarification, examples overhaul, adoc formatting

* adoc to md

* format change for rustdoc

* update links and fix typos

* typos and links

* updates according to comments

* new example

* small clarifications

* trait implementation section

* missing ```

* small changes, ready for review

* line width update

* small tweaks

* Update srml/balances/src/lib.rs

Co-Authored-By: joepetrowski <25483142+joepetrowski@users.noreply.github.com>

* Update srml/balances/src/lib.rs

Co-Authored-By: joepetrowski <25483142+joepetrowski@users.noreply.github.com>

* Update srml/balances/src/lib.rs

Co-Authored-By: joepetrowski <25483142+joepetrowski@users.noreply.github.com>

* Update srml/balances/src/lib.rs

Co-Authored-By: joepetrowski <25483142+joepetrowski@users.noreply.github.com>

* Update lib.rs

* address review by thiolliere

* remove common warning

* Update docs

* updated srml example
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.