Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do some rustdoc cleanups #1357

Merged
merged 7 commits into from Dec 7, 2022
Merged

Conversation

tcharding
Copy link
Member

Put some effort into our documentation, this is all pretty non-controversial. Patches separated to ease review.

Kixunil
Kixunil previously approved these changes Oct 31, 2022
Copy link
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 d9d1d84

This is a strict improvement but there are a few thing that could be even better. (For a future PR.)

bitcoin/src/blockdata/constants.rs Outdated Show resolved Hide resolved
bitcoin/src/blockdata/transaction.rs Outdated Show resolved Hide resolved
bitcoin/src/blockdata/transaction.rs Show resolved Hide resolved
bitcoin/src/blockdata/transaction.rs Outdated Show resolved Hide resolved
bitcoin/src/blockdata/transaction.rs Outdated Show resolved Hide resolved
bitcoin/src/blockdata/witness.rs Show resolved Hide resolved
bitcoin/src/blockdata/witness.rs Outdated Show resolved Hide resolved
@tcharding
Copy link
Member Author

Thanks for the review @Kixunil, I've added "do follow up PR" to my todo list.

@tcharding
Copy link
Member Author

Rebased to pick up recent clippy fixes. Added an additional patch containing a bunch of improvements as suggested in the review above.

Copy link
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.

The PR looks good, I'm not sure why it's a draft.

bitcoin/src/blockdata/transaction.rs Show resolved Hide resolved
bitcoin/src/blockdata/transaction.rs Outdated Show resolved Hide resolved
@tcharding
Copy link
Member Author

The PR looks good, I'm not sure why it's a draft.

So as not to force rebasing onto others like you mention above :) I believe this PR conflicts with #1323 so I'll let that one go in first.

@tcharding
Copy link
Member Author

Rebased, no other changes.

@tcharding tcharding marked this pull request as ready for review November 7, 2022 22:38
apoelstra
apoelstra previously approved these changes Nov 7, 2022
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 7ed91bd

@tcharding
Copy link
Member Author

Rebased only, no other changes.

bitcoin/src/blockdata/constants.rs Outdated Show resolved Hide resolved
@tcharding
Copy link
Member Author

Fixed the rebase fail.

apoelstra
apoelstra previously approved these changes Nov 23, 2022
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 9761e4a

rustdocs should contain a newline to separate the brief description from
the rest of long description.
Do an audit of the `blockdata` module and clean up rustdocs.
Reduce the number of lines of code by using a longer column width, 100
as is more-or-less standard in this repo.

This patch only changes column width (line length), no other changes.
The comments add no value to the code, remove them.
Do an audit of the `consensus` module and clean up rustdocs.
Recently we (tcharding) do some mechanical improvements to the rustdocs
in the `blockdata` module without considering the content. On review a
bunch of improvements were suggested.

Improve the content of various rustdoc comments in the `blockdata`
module.

Suggested content came from reviewers, all mistakes are my own :)
In order to really bring the security risks of RBF to peoples attention
make the docs more scary.
@tcharding
Copy link
Member Author

Rebased removing changes to max_money infavour of the current docs since we recently improved them anyways.

Copy link
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 26be9dd

The missing ns can go to typos issue.

fn emit_u16(&mut self, v: u16) -> Result<(), io::Error>;
/// Output a 8-bit uint
/// Outputs a 8-bit unsigned integer.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: an

fn emit_i16(&mut self, v: i16) -> Result<(), io::Error>;
/// Output a 8-bit int
/// Outputs a 8-bit signed integer.
Copy link
Collaborator

Choose a reason for hiding this comment

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

an

fn read_i16(&mut self) -> Result<i16, Error>;
/// Read a 8-bit int
/// Reads a 8-bit signed integer.
Copy link
Collaborator

Choose a reason for hiding this comment

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

an

fn read_u16(&mut self) -> Result<u16, Error>;
/// Read a 8-bit uint
/// Reads a 8-bit unsigned integer.
Copy link
Collaborator

Choose a reason for hiding this comment

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

an

@tcharding tcharding mentioned this pull request Dec 6, 2022
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 26be9dd

@apoelstra apoelstra merged commit fd5c5e4 into rust-bitcoin:master Dec 7, 2022
@tcharding tcharding deleted the 10-31-cleanups branch December 8, 2022 01:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants