Release 1.0-rc.0 #172
Release 1.0-rc.0 #172
Conversation
|
Are these conflicts real or a Github hiccup? Can you rebase on master? |
|
Err, rebase on 1.x? IIRC Tobin did a force-push to the 1.x branch which somehow changed the starting point. |
|
Oh, yeah, OK. |
Introduce decoding logic from `master`, this means: Public API consisting of: - The crate level decoding functions - The associated error types Private code: - Code in private modules `error` and `iter` as needed. This commit is a modification of Tobin Harding's commit. The relevant changes: * Renamed types to convey the difference as dynamic vs fixed sized rather than destination types (since `Vec<u8>` could be plausibly used for fixed-sized values). * Renamed functions to also convey the difference as dynamic vs fixed sized because we might at some point add a function that decodes fixed-sized data into `Vec<u8>` (to avoid storing large values on stack). * Deleted useless `FromHex` trait and code folded into `lib.rs` * Improved error messages with focus on the end user rather than developer. * More detailed documentation, especially regarding its purpose and the reason for the difference between `1.0` and `0.x.y`. * Removed reference to Bitcoin from the README (the library is general purpose, not Bitcoin-related). * Added `offset` method to all relevant error types - because we want to add multiple characters later, it's crucial to have this method available now so that libraries can start using it to adjust positions so that implementing multiple characters won't break anything. Even if we for some reason decide not to add it it's not only harmless but potentially useful regardless (downstream users don't need to define their own type just to communicate the offset). * Cleaned up feature gating on `std` - 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. * Added `new-rust-version` feature flag. Rationale: 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. However 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. * Removed `alloc`-related error types when the `alloc` feature is off. While not broken, they were completely useless without the feature and were just taking compilation time and doc space. * Added `.gitignore`. * Removed `Cargo.lock` - we have two other files for this purpose, this was likely committed by accident because of missing `.gitignore`.
In preparation for doing the first 1.0 rc release set the version number, add a changelog entry, and update the lock files. This is a commit made by Tobin Harding with the version changed from `alpha.0` to `rc.0` and extended change log.
Kixunil
force-pushed
the
release-1.0-rc.0
branch
from
7add9b5 to
4b15b7f
Compare
| /// | ||
| /// Errors if `hex` contains invalid characters or doesn't have even length. | ||
| #[cfg(feature = "alloc")] | ||
| pub fn decode_dyn_sized(hex: &str) -> Result<Vec<u8>, DecodeDynSizedBytesError> { |
There was a problem hiding this comment.
In 040a20e:
Both dyn and sized mean things in Rust that have nothing to do with this funtion. Calling this decode_vec was fine. You could also call it decode because the obvious thing that a hex string decodes to is a byte vector.
There was a problem hiding this comment.
Yeah, I'm not happy with that either. But is there a better way in English to express "this doesn't have fixed length"?
There was a problem hiding this comment.
A &str does not have a fixed length. Nor does a Vec<u8>. We don't need to express this. It's in the type signature, and the type signature is the most natural thing that any function called hex::decode could have.
If you don't like decode_vec I think we should go with decode.
There was a problem hiding this comment.
I'd rather go with _vec suffix to not make vecs more convenient than arrays.
| /// # Errors | ||
| /// | ||
| /// Errors if `hex` contains invalid characters or has incorrect length. (Should be `N * 2`.) | ||
| pub fn decode_fixed_sized<const N: usize>(hex: &str) -> Result<[u8; N], DecodeFixedSizedBytesError> { |
There was a problem hiding this comment.
In 040a20e:
sized means something in Rust that has nothing to do with this function. I guess you mean fixed_size, but then this is in contradiction with the commit message where you say that maybe we'll also want a method to decode fixed-size objects to Vec<u8>.
decode_array was fine. decode_fixed_size would also be ok, even though it's longer and less descriptive and might conflict with future functions.
There was a problem hiding this comment.
I definitely didn't mean size since that'd sound like we're decoding some kind of size.
Also it's not really that far off Rut terminology - arrays are Sized and whatever is backing a Vec is not.
There is no contradiction - if a function is needed we can add it later. If Box::write was stable I might've just ignored this and tell people to use it with this function since it should optimize well.
There was a problem hiding this comment.
If we use fixed_size it sounds like we're decoding a fixed-size object. It does not sound like we're decoding a size.
And Vecs are Sized. This terminology is confusing.
| /// ever pass in the position of the parsed hex string relative to the start of the entire | ||
| /// input. In that case overflow is impossible. | ||
| /// | ||
| /// This method is specifically designed to be used with [`map_err`](Result::map_err) method of |
There was a problem hiding this comment.
In ba658f2:
This line feels weird. You'd think if it were specifically designed for Result::map_err then it'd match the signature of the argument Result::map_err, which it can't because it takes extra data.
Better to just drop this line and trust that users are familiar with the Rust standard library.
There was a problem hiding this comment.
The point of the line is that this is why it returns Self rather than taking &mut, but maybe we should go all the way:
which it can't because it takes extra data.
There's an idiom I tend use sometimes:
fn add_error_context(context: Context) -> impl FnOnce(SomeError) -> MaybeDifferentError {
move |error| {
// do the conversion here
}
}Which actually can be used as an argument just fine: .map_err(add_error_context(context)). This is obviously not a good idea with contexts that are expensive to construct but fine with integers.
But because it's unusual (I have literally never seen anyone else doing this) that seems too much to me.
BTW, since the input is consumed, the current design makes modifying &mut quite annoying. But I think these kinds of mappings will be the only place where people will reasonably use this.
There was a problem hiding this comment.
Heh, yeah, that did occur to me after I'd posted my comment. But agreed, it's too unusual and almost looks like a mistake when it's used, so better to just stick with the common (if ugly) thing.
|
I might revert the function names to |
|
Sorry, lot of other shit lately. I'm pretty sure the array name is fine, vec, I'm not sure but willing to rename it back to make progress. I'll try to update the PR today/tomorrow. |
|
Why is all the new stuff not being pushed to master first? |
|
If I am understanding patch 1 (ba658f2) it conceptually does a few things:
So I don't see why this all is not PR'd against master then this PR becomes basically a simple copy paste of files on master. |
|
@tcharding oh, I've misunderstood the process. I was just going to do the renames now but seeing that I'll look into making a PR to master. |
|
I also misunderstood the process. I had thought we were going to publish stuff on 1.x and then just add re-exports from 1.x on master without ever having the "real code" on master. Your way does lead to a clearer history. |
d73c686 Rename error types to convey their semantics (Martin Habovstiak) 8386dc6 Improve crate documentation (Martin Habovstiak) 6170d45 Add `newer-rust-version` feature (Martin Habovstiak) 69bd229 Clean up feature gating on `std` (Martin Habovstiak) 87f723d Add `offset` method to relevant error types (Martin Habovstiak) 31cece0 Hide `invalid_char` method (Martin Habovstiak) 9b3ad8a Expose the error types as enums directly (Martin Habovstiak) 9b5691e Make the error messages more user-oriented (Martin Habovstiak) 8698ed3 Rename `decode_{ty}` functions to `decode_to_{ty}` (Martin Habovstiak) fc8958b Swap decoding implementations (Martin Habovstiak) 7e49ee2 Add Policy section to MSRV in README (Martin Habovstiak) aa0cd91 Fix Githooks heading level in README (Martin Habovstiak) 19dd213 Remove reference to Bitcoin from README (Martin Habovstiak) Pull request description: This includes the relevant stuff from #172 along with the changes requested in the review. ACKs for top commit: tcharding: ACK d73c686 apoelstra: ACK d73c686; successfully ran local tests Tree-SHA512: c2212d766b9b6e342c4913f5c37fa6137c1b86770cdb04bbaabd7fbab2450c34d2a883269f41e0dc79227d397987df77ae8e6a1f1e5a5a89c8d9480299ab20dc
|
I've pushed two patches to the The commit log in ba658f2 is stale, needs a bunch of stuff removing since we merged into master. |
|
Explicitly leaving docs fixes for later on. I only found a couple of super trivial ones anyways. LFG! |
|
I'm kinda confused what should the next steps be. The PR merged into I'm thinking I should mostly just copy the files and apply relevant patches but maybe you had a different process in mind? |
|
IMO this is ready for release merge and release. @apoelstra can you cut the release please? |
|
Looking at the diff between
So yes, I'm technically fine with the rc, though I will try to clean it up if possible. I plan to open a new PR so that either it gets reviewed in time and can replace this one or it doesn't and this one can go in. |
|
Hiya. Sorry for ignoring this. My parents were over last week and I built up a backlog I had to get through. Lemme review this and let's go. |
|
Ok, NACK the name |
Oh, interesting, I though statically-sized is proper English. |
|
"Statically-sized" is okay. "Fixed-sized" is okay if you interpret "fixed" as an adverb modifying the adjective "sized" but it's not the natural way to read it, because
|
It's not, it's called
|
|
@Kixunil can you remake this PR by just taking |
|
Yeah, I will try to look into it tomorrow. |
|
I am going to do the followups to rust-bitcoin/rust-secp256k1#716 and then I'm going to take this on. So today or tomorrow. (And yes, I know that I'm no more likely than you to actually hit a "tomorrow" deadline :) but let's get this thing done!) |
|
LFG |
|
Closing in favor of #175 |
Alternative to #162 with a bunch more cleanups and improvements.
I'm fairly confident this is as close to releasable as I can get, however please do check the API and please do complain if you see a problem.
If you see problems unrelated to the API please say so but we can fix them between 1.0.0-rc.0 and 1.0.0 so I'll only apply such fixes if there are no ACKs and I have enough time.