Cleanups to prepare 1.0 release #173
Conversation
Despite being maintained by rust-bitcoin developers this library is general-purpose. There's nothing specific to bitcoin in this library and, except for reversed display of hashes, there's nothing special about hex usage in Bitcoin. (And reversing doesn't need special support from this library.)
Githooks have nothing to do with MSRV so information about them should not be sub-section of MSRV.
Kixunil
added
documentation
Kixunil
force-pushed
the
1.0-cleanups
branch
4 times, most recently
from
90cec1e to
579bdff
Compare
README.md
Outdated
| Note though that the dependencies may have looser policy. This is not considered breaking/wrong | ||
| - you would just need to pin them in `Cargo.lock` (not `.toml`). |
There was a problem hiding this comment.
This renders as a list because the - is at the start of the line
Seen here: https://github.com/Kixunil/hex-conservative/blob/1.0-cleanups/README.md
src/error.rs
Outdated
| /// This error differs from `DecodeFixedSizedBytesError` in that the number of bytes is only known at run time | ||
| /// - e.g. when decoding `Vec<u8>`. |
There was a problem hiding this comment.
This renders as a list item because of the - at the start of the line.
There was a problem hiding this comment.
We could use a link for DecodeFixedSizedBytesErro after 'differs from' since reader is likely to want to see the DecodeFixedSizedBytesError when reading.
There was a problem hiding this comment.
This fixes both things
/// This error differs from [`DecodeDynSizedBytesError`] in that the number of bytes is known at
/// compile time - e.g. when decoding to an array of bytes.
|
With the docs fixes: ACK 579bdff |
The README claims that we have a conservative MSRV policy but doesn't specify how. This commit adds clarification how we intend to bump the MSRV.
We'd like to limit the API in 1.0 to the decoding functions in crate root and omit the `FromHex` trait to make the API smaller and easier to maintain. Because the trait won't be in 1.0, the functions should not call into it. This moves the implementation of decoding from the trait methods to free-standing functions and calls those functions inside trait methods.
The functions `decode_vec` and `decode_array` sounded like they are decoding *from* those objects rather than *to* them. Similar crates that have such free-standing functions refer to the source in their name but we refer to destination. This renames them so that they contain `_to` making it obvious it's the destination not the source.
The messages produced by `Display` impl will be read by the end users who may not know anything about programming. This change makes sure to style them nicely and explain more clearly what is the problem with the input.
The error types in the form of `Outer(pub(crate) Inner)` where `Inner` needs to be public and accessible anyway have a few problems: * Understanding two types is harder than understanding just one type * They are more annoying for the consumers to match on * They take up space in documentation * Naming them is difficult Hex decoding should be in principle easy and all possible error types are fixed, so there shouldn't be future changes. Adding a common field can still be done simply by adding it to each variant. This even has the advantage that newly added field is not lost during some conversions (e.g. if downstream crate knows the length is correct). If fast access is needed we can just arrange all structs to place the common fields at the same offset (e.g. beginning). So I only see two cases where modifying them would be needed: * Adding a `PhantomData` field - this is highly unlikely in this crate, there's no context. We could have the char type generic in the future (`char`, `u8`, `core::ascii::Char`) but that would be stored inside `InvalidCharError`, so no need for `PhantomData`. * Making the error boxed internally - we can also just box the inner errors individually and it'll only cost an extra `usize` for discriminant making the entire error type two `usize`s. This is already smaller than `Vec`, so returning it from `decode_to_vec` is non-issue and callers can add another layer of box if needed. It is worse when returning short arrays but those seem to be rare. I think that we can do a lot of layout fiddling and the compiler can do some too, so I think it'll work fine. And any application that is so performance-sensitive that 1 `usize` is going to make a difference should probably not parse hex to begin with. Thus I believe it's worth just exposing the errors and this commit implements it.
We want to eventually return `char` instead so this hides the method until that is fixed.
When a larger string is parsed that may consist of multiple parts, some of them being hex strings, the returned errors should carry the position relative to the start of the entire string. The design of errors so far required users to create a new type which would either carry the position of the hex string or lose some information by converting from our types. This includes a problem that if a different crate in the tree enables a feature to find all invalid characters the calling crate has no way to know about it and has to lose the remaining characters. This can be solved by providing a method to modify all the positions stored inside the error type. Such method should be added ASAP so that all libraries can start using it right away. Thus we want to include this before 1.0 release. This commit adds the method, taking into account that it will be used inside `map_err` quite very often.
Using `not(feature = "std")` makes modifications frustrating because the feature internally behaves as if it wasn't additive leading to complex conditions. It's better to just set `#[no_std]` unconditionally and then optionally reference the additional features.
|
I fixed the docs as suggested. |
There was a problem hiding this comment.
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (3)
tests/api.rs:89
- [nitpick] Consider using more descriptive field names in the Errors struct instead of single letters (e.g., 'hex_array_error' instead of 'c') to improve clarity in test output.
struct Errors {
src/error.rs:241
- [nitpick] The custom Display implementation for InvalidCharError uses nested helper structures to adjust the output and could be simplified. Consider extracting the formatting logic into a separate helper to improve readability.
write!(f, "the {} character, {}, is not a valid hex digit", which, chr)
Cargo.toml:30
- Ensure that the addition of the 'newer-rust-version' feature and its dependency is well documented and covered by tests to verify that feature detection behaves as expected under various toolchain versions.
newer-rust-version = ["dep:if_rust_version"]
We've decided that we want to use the `if_rust_version` crate to avoid compiling and running the same build script multiple times across many crates. However, currently the additional features are just about `core::error::Error` and thus irrelevant for `std` users (majority). Depending on it unconditionally would just add dependency.Making the crate optional is a natural solution. This change imlpements it. The specific crate used is considered an internal detail and this will even go away once the native solution is in MSRV. Thus we rename the feature to better convey the meaning. It is also documented that this feature may or may not add dependency and, since we have a way of removing it, it's also documented what will happen once the feature becomes irrelevant.
With upcoming decoding stabilization we need to inform users about details of the stabilization and make sure our API doc is understandable. This makes various doc improvements including explaining how we're going to stabilize things and feature flags.
If a downstream library decodes a hex string into a particular type using a function in this library the returned error type would "leak" which type the library used internally. The specific type shouldn't really concern the callers since it can change anyway. What is relevant though is whether the length is known at compile time or not. We might also add more functions in the future decoding to different types and it'd be better to share error types in such case. This change renames `HexToBytesError` to `DecodeDynSizedBytesError` and `HexToArrayError` to `DecodeFixedSizedBytesError` so that types are not mentioned.
There was a problem hiding this comment.
Copilot reviewed 12 out of 13 changed files in this pull request and generated no comments.
Files not reviewed (1)
- contrib/test_vars.sh: Language not supported
Comments suppressed due to low confidence (1)
src/error.rs:211
- [nitpick] The uninitialized variable 'which' used to build the display output for InvalidCharError can be confusing. Consider refactoring this match arm to inline the creation of the formatted output or using a more descriptive variable name to improve clarity.
let which;
|
Nice! I'm happy with these names. Am in flight today but will review this now so that it can get into the queue before I'm offline. Disappointing that copilot is still useless. |
|
Will ACK and merge this regardless, but I'd like a followup to
I can do this. It will be a much smaller PR than this one. |
It did find two issues without me having to do anything other than clicking. I consider it a win. Maybe it's value is limited but still, I didn't have to wait for you to find those mistakes and fixed them quickly.
I disagree with putting specific version in the feature name. Currently it's only about
As I explained elsewhere, it is very similar to |
|
I strongly disagree that there is any similarity between I guess if users were hex-encoding the raw byte patterns of Rust objects (without following any internal pointers) the terms would coincide. In any case, I'll PR to change the names and we can debate there. |
|
|
|
You can argue that |
|
They seem extremely similar to me. 🤷 |
3 participants
This includes the relevant stuff from #172 along with the changes requested in the review.