-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Documentation for the timestamp module #1927
Conversation
gautamdhameja
commented
Mar 5, 2019
- Added code comments in the lib.rs of the timestamp module
- Added readme.md in the root of timestamp module crate. This file has the module docs as per the SRML documentation template.
|
It looks like @gautamdhameja signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
1 similar comment
|
It looks like @gautamdhameja signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
d427dac to
679ec03
Compare
|
Whenever I wanted to learn about a new srml, lack of explanation on the test cases was quite a barrier. I would recommend putting some explanatory docs on the tests as well since they should cover the common use cases of the module + common error/edge cases. Hence, I think they are quite important and should be documented + potentially linked as a further source of learning at the end of the .md file. |
|
Is there any opportunity to incorporate: https://github.com/paritytech/substrate/blob/master/README.adoc#72-contributing-to-documentation-for-substrate-packages |
ltfschoen
left a comment
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.
great work so far! i've left some nitpick comments
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.
some general thoughts:
in rustdoc to make link you can write [NameOfTheStuff] and then at the end of the doc after an empty line you write [NameOfTheStuff]: struct.NameOfTheStuff.html
look at ./core/sr-sandbox/src/lib.rs line 79 for example
using this method a lot of types can be referenced. like block_period, set, get etc...
also I'm not sure some /// doc on private function with expressfull name is useful.
You don't need to do this anymore, just write [ |
it seems to be the case on nightly only |
shawntabrizi
left a comment
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 have 2 suggestions, but nothing that should block a merge
ltfschoen
left a comment
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.
LGTM, left a couple more nitpick
|
🥇 |