Improve documentation #340
Conversation
|
concept ACK. This PR touches a lot of code and is likely to get quickly out-of-date (and conversely to invalidate lots of open PRs). For that reason I'm hesitant to move forward with it, but I'm happy to try if others want to. |
|
Not sure what to do about this. I'll put it back to draft until some idea comes to me. Cheers. |
|
I'm reopening this because we have a bit of a quiet time right now, there are not many PRs open for this to conflict with, possibly only @dr-orlovsky's #309? |
'context' does not need need a capital letter in the middle of a sentence.
Add the terminating period to all docs sentences. (Also one instance of capitialize initial character in sentence.)
Make all the various size constant docs uniform by using form 'The size ...' and also by ending with a period.
As is typical in the Rust ecosystem use the third person tense when documenting functions. E.g., ``` /// Creates a new Foo. ``` As opposed to ``` /// Create a new Foo. ```
For added clarity add ticks around words that are code.
To save devs looking up the commit themselves add a link to it in the rustdoc.
Recently we introduced uniform styling for module docs over in `rust-bitcoin` repo. We can do the same here but its a bit controversial because it removes the heading from module docs and every single public module in rust-secp256k1 uses a heading. Instead we use a full sentences. Also makes uniform the trailing `//!`.
src/context.rs
Outdated
| /// A trait for all kinds of Context's that lets you define the exact flags and a function to deallocate memory. | ||
| /// It shouldn't be possible to implement this for types outside this crate. | ||
| /// A trait for all kinds of contexts that lets you define the exact flags and a function to | ||
| /// deallocate memory. It shouldn't be possible to implement this for types outside this crate. |
There was a problem hiding this comment.
In 269bde0:
You could change "shouldn't be possible" to "isn't possible". I think we've come to trust rustc more than when this comment was written :)
src/context.rs
Outdated
| // on one hand this trait is public, on the other it's in a private module | ||
| // so it's not visible to anyone besides it's parent (the context module) | ||
| // On one hand this trait is public, on the other it's in a private module | ||
| // so it's not visible to anyone besides it's parent (the context module). |
There was a problem hiding this comment.
In 5e07e75:
Should also change the second it's to its
There was a problem hiding this comment.
Also we could probably drop the exposition here; it's a pretty standard idiom now.
src/lib.rs
Outdated
| /// (Re)randomizes the Secp256k1 context for cheap sidechannel resistance; | ||
| /// see comment in libsecp256k1 commit d2275795f by Gregory Maxwell. Requires | ||
| /// compilation with "rand" feature. | ||
| /// (Re)randomizes the Secp256k1 context for cheap sidechannel resistance. |
There was a problem hiding this comment.
In 3fa6762:
Let's replace "cheap" with "extra". The cost is comparable to a signing operation.
|
Done reviewing c9e6ca1 I think it's a quiet enough time that we can get this in, it's not that disruptive and we're mostly done with Schnorr signatures. |
This definitely isn't possible, change the phrase.
The nested pub inside a private module is easy to understand, we do not need an explanation.
The word 'extra' better describes the sidechannel resistance gained by re-randomising the context.
|
All suggestions implemented as separate additional patches, thanks. |
In a continued effort to find my feet around here, and inspired by issue #128 I've done a codebase wide audit of the docs (primarily just rustdocs but I glanced at
//docs as well). Each change is in a separate commit so can be removed if resistance is met. ("resistance is futile").I've based the stylistic decisions on work done in rust-bitcoin.
I believe the only controversial change is the last (commit: da161c9 Use rust-bitcoin module doc style), please review that one carefully.