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
hashes: make hex optional #2711
Draft
tcharding
wants to merge
15
commits into
rust-bitcoin:master
Choose a base branch
from
tcharding:04-23-hashes-make-hex-optional
base: master
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
hashes: make hex optional #2711
tcharding
wants to merge
15
commits into
rust-bitcoin:master
from
tcharding:04-23-hashes-make-hex-optional
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We would like to check for API changes during development and in CI so that such changes can be discussed separately from the code. We have API listings already. Add tooling and documentation for creating API listings for the public crates within the workspace (i.e., not `internals`). Add API text files from an initial run of the script.
Add a job that runs the new script to check for changes to the public APIs of `hashes` and `bitcoin`.
Add a `just` command to run the API checking script. Makes it more discoverable.
Run `just check-api`, no other manual changes.
Currently we use `hashes::hash_newtype!` to create various types that wrap hash types. While this works it makes the `hashes` crate less maintainable and more prone to change because logic that we want to control from `bitcoin` is in `hashes`. We want to stabalise the leaf crates as quickly as possible, moving the `hash_newtype` logic to `bitcoin` allows us to simplify `hashes` and put the code closer to where it is used. Note, this patch does not remove `hashes::hash_newtype`, it just adds a bunch of additional macros to `bitcoin::internal_macros`. Note on design: Instead of a single enormous macro, we add multiple smaller macros. This allows implementing just what is needed for a type and gives finer control over what is implemented and what is in the public API. All at the cost of being a bit more verbose.
Show what changed with after removing usage of `hashes::hash_newtype!`.
As we did for the `hashes::hash_newtype!` macro; replace the `hashes::sha256t_hash_newtype!` macro with a collection of more simple macros in the `bitcoin` crate. This improves maintainability by giving putting control of the macro logic in the crate that uses the macro. Also assists in stabalizing `hashes` sooner because we can now remove the two macros from `hashes`.
The `hashes` crate is _hard_ to work on, every time one wants to change something there are various macros that are far from trivial to understand. We would like to improve the `hashes` API so we can stabalize the crate. In an effort to assist in doing so remove the two hash newtype macros. This is obviously an API breaking change - is anyone outside of `rust-bitcoin` using these macros, if so they are going to be annoyed with us. Remove the `hash_newtype!` and `sha256_hash_newtype` macros.
As can be seen from the macros in `bitcoin` we can fully support tagged SHA256 hashes without needing a module in `hashes`. In an effort to make the `hashes` crate easier to work on remove the `sha256t` module all together. This risks rugging and annoying users but either I need 10 more points of IQ to work on the `hashes` crate or we have to do something to reduce the complexity.
This macro is no longer used within this crate or in `bitcoin` - lets remove it. Done in preparation for making `hex` an optional dependency. This patch is an unnecessarily brutal change, we could deprecated for a release cycle instead.
github-actions
bot
added
ci
C-bitcoin
PRs modifying the bitcoin crate
C-hashes
PRs modifying the hashes crate
doc
labels
Apr 23, 2024
Pull Request Test Coverage Report for Build 8797148222Details
💛 - Coveralls |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Done on top of #2708, just a showcase to share the serendipity I felt just now.
hashes::hex_fmt_impl
hex
optional - and the patch is trivial!Close: #2654