-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Conversation
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
Suggest reviewing latest comments in #1927. |
@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. |
Latest commit for conflict resolution after #2048 |
lots of changes since review
//! | ||
//! The `Call` enum is documented [here](https://crates.parity.io/srml_balances/enum.Call.html). | ||
//! | ||
//! - `transfer` - Transfer some liquid free balance to another 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 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
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.
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.
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>
//! } | ||
//! } | ||
//! } | ||
//! ``` |
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 kind of agree for this example but at the same time it is not specific to balance, so I'm mixed but ok
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.
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.
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'll approve since there's nothing wrong with committing as-is, but you might want to bring back the contracts example.
//! 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)?; |
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 think the updated function name is transfer()
instead of make_transfer
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.
Yeah, and more generally throughout some of the early docs whose modules had a refactor. #2159
* 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
First attempt at documentation for the balances module. Besides a general review, this needs:
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.