Skip to content

util/address: Improve docs#704

Merged
dr-orlovsky merged 3 commits intorust-bitcoin:masterfrom
tcharding:rustdocs-address
Nov 23, 2021
Merged

util/address: Improve docs#704
dr-orlovsky merged 3 commits intorust-bitcoin:masterfrom
tcharding:rustdocs-address

Conversation

@tcharding
Copy link
Copy Markdown
Member

@tcharding tcharding commented Nov 16, 2021 •
edited
Loading

Improve documentation of the address module by doing:

  • Add full stops to all sentences
  • Use code ticks even inside links e.g., [WitnessVersion]
  • Use 100 character line length
  • Do grammar fixes
  • Use comment sections (e.g. # Returns)
  • Use 3rd person for function comments e.g. 'Converts foo to bar' instead of 'Convert foo to bar'
  • Use ticks for scriptPubkey

This patch does a single file because a bunch of these changes pick an
arbitrary stlye, if we can bikeshed on this PR then future PRs should be
able to progress more quickly. I'll take lack of comment on any of the
above as approval and I'll attempt to be uniform when doing the rest of
the codebase. I plan on just chipping away at this, I can only do so
much docs work in a day without getting bored of it :)

Notes:

  • I didn't touch 'segwit' vs 'SegWit', seems both are widely used.
  • Using ticks inside links may be an overkill but seems more correct?
  • I'm not totally sure where the line is in the Rust ecosystem between
    readability in an editor and rendering as HTML, open to input on this.

@Kixunil
Copy link
Copy Markdown
Collaborator

Kixunil commented Nov 17, 2021

Use imperative tense for function comments

The other sounds more natural to me as it describes what the function does. Can you explain why is this change useful?

Kixunil
Kixunil reviewed Nov 17, 2021
View reviewed changes
Kixunil
Kixunil reviewed Nov 17, 2021
View reviewed changes
@Kixunil
Copy link
Copy Markdown
Collaborator

Kixunil commented Nov 17, 2021

I agree to or don't care about the remaining changes.

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Nov 17, 2021

Use imperative tense for function comments

The other sounds more natural to me as it describes what the function does. Can you explain why is this change useful?

Drats! When I wrote the commit log and I couldn't come up with a reference for this change I knew it would come back to bite me :)

I dug more into the topic, I could not find anyone mentioning it specifically in relation to Rust only to other languages so I looked at the following 'random' crates, all of them use 3rd person not imperative.

  • a bunch of std lib crates
  • log
  • openssl
  • tokio

So it looks like I'm wrong in doing this change but I learned something in the process. I will remove these changes and make any other function docs that get edited use the 3rd person.

Thanks for the review @Kixunil.

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Nov 17, 2021

I elected to force push with your suggestions implemented @Kixunil, the diff is not very long and in the name of making it easier for others to give their opinions I don't think I've made it much harder for you to re-review if you so desire. Thanks again!

@tcharding
util/address: Improve docs
eb8278f 
Improve documentation of the `address` module by doing:

- Add full stops to all sentences
- Use code ticks even inside links e.g., [`WitnessVersion`]
- Use 100 character line length
- Do grammar fixes
- Use comment sections (e.g. `# Returns`)
- Use 3rd person for function comments e.g. 'Converts foo to bar' instead of 'Convert foo to bar'
- Use ticks for scriptPubkey

This patch does a single file because a bunch of these changes pick an
arbitrary stlye, if we can bikeshed on this PR then future PRs should be
able to progress more quickly. I'll take lack of comment on any of the
above as approval and I'll attempt to be uniform when doing the rest of
the codebase. I plan on just chipping away at this, I can only do so
much docs work in a day without getting bored of it :)

Notes:

- I didn't touch 'segwit' vs 'SegWit', seems both are widely used.
- Using ticks inside links may be an overkill but seems more correct?
- I'm not totally sure where the line is in the Rust ecosystem between
  readability in an editor and rendering as HTML, open to input on this.
@tcharding tcharding mentioned this pull request Nov 17, 2021
Closed
Kixunil
Kixunil previously approved these changes Nov 17, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

Did a bit deeper review and found a few more improvement opportunities but given that this is strictly better than before:

ACK eb8278f

@tcharding tcharding mentioned this pull request Nov 18, 2021
Merged
@Kixunil Kixunil added code quality Makes code easier to understand and less likely to lead to problems documentation P-low Low priority labels Nov 18, 2021
@tcharding
Improve documentation of WitnessVersion
804a38c 
Attempt to improve the rustdocs for `WitnessVersion` in line with
review comments from a previous patch.
@tcharding
Improve constructor rustdocs for Address
822c992 
Improve the rustdocs for the various `Address` constructors by putting
the brief description on a separate line with further description in its
own paragraph. This is the layout best practice for function documentation
using rustdocs.

Also, favour 'creates' over 'constructs' because it is more common in
the docs of this struct.
@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Nov 18, 2021

I've done both your remaining suggestions @Kixunil as separate patches. Thanks.

Kixunil
Kixunil approved these changes Nov 18, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK 822c992

dr-orlovsky
dr-orlovsky approved these changes Nov 23, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

ACK 822c992

@dr-orlovsky dr-orlovsky merged commit d614b6c into rust-bitcoin:master Nov 23, 2021
@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented Nov 24, 2021

belated concept ack

@tcharding tcharding deleted the rustdocs-address branch November 25, 2021 22:07
@dr-orlovsky dr-orlovsky mentioned this pull request Jan 15, 2022
Closed
apoelstra added a commit to rust-bitcoin/rust-secp256k1 that referenced this pull request Feb 10, 2022
@apoelstra
Merge #340: Improve documentation
df7520e 
c73eb2f Use 'extra' instead of 'cheap' (Tobin Harding)
c79eb97 Remove unnecessary explanation (Tobin Harding)
f95e91a Use isn't instead of shouldn't (Tobin Harding)
c9e6ca1 Use rust-bitcoin module doc style (Tobin Harding)
3fa6762 Add link to referenced commit (Tobin Harding)
f5e68f3 Add ticks around code snippet (Tobin Harding)
d25431c Use 3rd person tense for function docs (Tobin Harding)
c3be285 Fix size constant docs (Tobin Harding)
5e07e75 Add period to sentences (Tobin Harding)
269bde0 Remove unnecessary capitalisation (Tobin Harding)

Pull request description:

  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](rust-bitcoin/rust-bitcoin#704) 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.

ACKs for top commit:
  apoelstra:
    ACK c73eb2f

Tree-SHA512: 5ea215de3fd23ca2a4f25d8f8d59a85a299044fe495269c43b621291ea50c58856fa8544e36cc109b7bdb1a7a59bcab8711f30113572ddce4509d3b06ff0d3b6
chain-forgexcr45 added a commit to chain-forgexcr45/rust-secp256k1 that referenced this pull request Sep 28, 2025
@chain-forgexcr45
Merge rust-bitcoin/rust-secp256k1#340: Improve documentation
a5785d7 
c73eb2f391d797256fb09545584c998d723d6f72 Use 'extra' instead of 'cheap' (Tobin Harding)
c79eb976ca4c5cd02275b4f3c843380f64f563b7 Remove unnecessary explanation (Tobin Harding)
f95e91a6dac2b585e868910119e22599b242911e Use isn't instead of shouldn't (Tobin Harding)
c9e6ca168000aa3b0aea128048a7f7a40986398c Use rust-bitcoin module doc style (Tobin Harding)
3fa67624375ba529ad37f7786ca759224ff139fd Add link to referenced commit (Tobin Harding)
f5e68f3ba76d734d9ef2a4ef9cad89879622958a Add ticks around code snippet (Tobin Harding)
d25431c1da9e290518e9e6aaace8c882fd047edb Use 3rd person tense for function docs (Tobin Harding)
c3be285c1dbad037609f9f11a61c4aa12d6f370e Fix size constant docs (Tobin Harding)
5e07e7596b3c128c2290089f87a2851946fb538d Add period to sentences (Tobin Harding)
269bde042f9d6bfccb857c6921e71f01a9854f35 Remove unnecessary capitalisation (Tobin Harding)

Pull request description:

  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](rust-bitcoin/rust-bitcoin#704) 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.

ACKs for top commit:
  apoelstra:
    ACK c73eb2f391d797256fb09545584c998d723d6f72

Tree-SHA512: 5ea215de3fd23ca2a4f25d8f8d59a85a299044fe495269c43b621291ea50c58856fa8544e36cc109b7bdb1a7a59bcab8711f30113572ddce4509d3b06ff0d3b6
william2332-limf added a commit to william2332-limf/rust-secp256k1 that referenced this pull request Oct 2, 2025
@william2332-limf
Merge rust-bitcoin/rust-secp256k1#340: Improve documentation
4bfebfb 
c73eb2f391d797256fb09545584c998d723d6f72 Use 'extra' instead of 'cheap' (Tobin Harding)
c79eb976ca4c5cd02275b4f3c843380f64f563b7 Remove unnecessary explanation (Tobin Harding)
f95e91a6dac2b585e868910119e22599b242911e Use isn't instead of shouldn't (Tobin Harding)
c9e6ca168000aa3b0aea128048a7f7a40986398c Use rust-bitcoin module doc style (Tobin Harding)
3fa67624375ba529ad37f7786ca759224ff139fd Add link to referenced commit (Tobin Harding)
f5e68f3ba76d734d9ef2a4ef9cad89879622958a Add ticks around code snippet (Tobin Harding)
d25431c1da9e290518e9e6aaace8c882fd047edb Use 3rd person tense for function docs (Tobin Harding)
c3be285c1dbad037609f9f11a61c4aa12d6f370e Fix size constant docs (Tobin Harding)
5e07e7596b3c128c2290089f87a2851946fb538d Add period to sentences (Tobin Harding)
269bde042f9d6bfccb857c6921e71f01a9854f35 Remove unnecessary capitalisation (Tobin Harding)

Pull request description:

  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](rust-bitcoin/rust-bitcoin#704) 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.

ACKs for top commit:
  apoelstra:
    ACK c73eb2f391d797256fb09545584c998d723d6f72

Tree-SHA512: 5ea215de3fd23ca2a4f25d8f8d59a85a299044fe495269c43b621291ea50c58856fa8544e36cc109b7bdb1a7a59bcab8711f30113572ddce4509d3b06ff0d3b6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Kixunil Kixunil Kixunil approved these changes

+1 more reviewer

@dr-orlovsky dr-orlovsky dr-orlovsky approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

code quality Makes code easier to understand and less likely to lead to problems P-low Low priority

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tcharding @Kixunil @apoelstra @dr-orlovsky