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.

Sign up for GitHub

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

Jump to bottom

Cleanup block module #864

Merged
merged 3 commits into from Mar 12, 2022
Merged

Cleanup block module #864

merged 3 commits into from Mar 12, 2022

Conversation

tcharding
Copy link
Member

Do some clean ups to the blockdata::block module.

  • Patch 1: Change predicate names (API breaking, could be seen as unnecessarily changing the API), can remove if NACK'd
  • Patch 2: Refactor to assist code clarity
  • Patch 3 and 4: are docs improvements, shouldn't be too controversial

@apoelstra
Copy link
Member

Honestly I prefer the old names. check_X is clear that it's checking whether the data X in the header is correct. X_matches implies that maybe one or the other could be wrong, and it's not clear what "the other" even is since these methods take no inputs.

@tcharding
Copy link
Member Author

Fair points. Will remove the rename check_ patch from this PR.

@tcharding
Refactor check_witness_commitment
5464848 
Currently function contains nested `if` clauses that arguably obfuscate
the code. We can make the code easier to read by pulling out the error
paths and returning them higher up in the function.

Refactor only, no logic changes.
@tcharding
Fix erroneous function rustdoc
f03092c 
The returns part of the function docs appears to be stale, remove it.
Improve wording of rustdocs while we are at it.
@tcharding
Improve docs for blockdata::block
146d5e8 
Improve the rustdocs for the `blockdata::block` module:

- Use full sentences (capitalisation and full stop)
- Use third person tense instead of imperative
- Improve wording if needed
@tcharding
Copy link
Member Author

Changes in force-push

  • Rebased on master
  • Removed the patch changing predicate names

apoelstra
apoelstra approved these changes Mar 10, 2022
View reviewed changes
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 146d5e8

dr-orlovsky
dr-orlovsky approved these changes Mar 12, 2022
View reviewed changes
Copy link
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 146d5e8

@dr-orlovsky dr-orlovsky merged commit 7e755d3 into rust-bitcoin:master Mar 12, 2022
@tcharding tcharding deleted the cleanup-block branch March 15, 2022 03:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@apoelstra apoelstra apoelstra approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tcharding @apoelstra @dr-orlovsky