Add u5 type and traits #17
Conversation
src/lib.rs
Outdated
| @@ -50,41 +50,140 @@ use std::{error, fmt}; | |||
| use std::str::FromStr; | |||
| use std::fmt::{Display, Formatter}; | |||
|
|
|||
| #[cfg(feature = "try_from")] | |||
There was a problem hiding this comment.
What is the compatibility of try_from? Other changes were made to this library to push compatibility back to 1.14.
There was a problem hiding this comment.
AFAIK it's new in 1.25, that's why I put it behind a feature gate. You can use it if your rustc version is new enough, but you can ignore it otherwise. Unfortunately I couldn't find a way to express "only activate if rustc version >= 1.25.0", so the user has to activate it manually. But this reminds me of a TODO: run the stable test with the feature activated and add a test for TryFrom.
EDIT: My reasoning behind this is: with the next debian release a new rustc version will be included. If new traits are already implemented it becomes much easier to upgrade the API: just deprecate and at some point remove the old methods.
There was a problem hiding this comment.
Although some stabilization PR for TryFrom was merged recently it seems not to work on 1.25. I will remove the feature.
src/lib.rs
Outdated
| /// Integer in the range `0..32` | ||
| #[derive(PartialEq, Debug, Copy, Clone, Default)] | ||
| #[allow(non_camel_case_types)] | ||
| #[repr(C)] |
There was a problem hiding this comment.
remove repr(C), was used to do tests with std::mem::transmute
|
You've got a few |
|
I will defineatly address them, that's why I marked the PR as WIP. But right now I'm using the API with bech32-bitcoin and lightning-invoice to find rough edges. I already found some and will fix them next. |
|
Imo the PR is ready for review now. I have two naming decisions that I'm not completely sure about:
|
|
Can you squash down the TODO fixes? Note that you can selectively squash with |
|
I could squash all of the commits if you wanted, but IMO a merge will hide the commits behind one and everyone interested can still look at this PR. |
|
It's very hard to review changes where commits introduce things that other commits remove. |
sgeisler
force-pushed
the
u5
branch
2 times, most recently
from
100fb7a
to
2e39a80
Compare
+ add to_u8 function to u5
|
With some squashing and rebasing I cut the commit count down to four. I hope this is a better starting point for a review. |
|
I will try to review soon. |
|
@apoelstra Any comments before we merge? |
|
I just saw that I didn't increase the version yet. I guess it would be 0.4.0? EDIT: and I need to sync the README with the examples from the docs. |
* set version to 0.4.0 * use examples from rustdoc in README
|
👍 I was thinking about that earlier. Thanks |
Fixes #16. I'm not really sure if the API is as good as it could be. I will have a look at rust-bech32-bitcoin and prepare a PR to fix any emerging incompatibilities. Maybe this will show me some rough edges which I can improve.