-
Notifications
You must be signed in to change notification settings - Fork 30
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
Remove requirement on bitvec for std feature and constructing TypeDefBitSequence. #168
Conversation
@@ -25,7 +25,7 @@ scale = { package = "parity-scale-codec", version = "3", default-features = fals | |||
[features] | |||
default = ["std"] | |||
std = [ | |||
"bitvec/std", | |||
"bitvec?/std", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bitvec crate will no longer be pulled in when "std" is enabled, but this I think makes no difference to the external interface since bitvec additions are behind the separate "bit-vec" feature anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that bitvec
will still ultimately be pulled in via scale/std
https://github.com/paritytech/parity-scale-codec/blob/b5f2b360e2939e8a235423f40591db09e879fdbb/Cargo.toml#L41
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh ok; I'll do a quick follow PR there then to prevent that :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR for parity-scale-codec thing: paritytech/parity-scale-codec#375
/// With the `bit-vec` feature enabled, the expected usage is to provide either | ||
/// `bitvec::order::Lsb0` or `bitvec::order::Msb0` as the order type, and then something | ||
/// like u8, u8, or u32 as the store type. Without the `bit-vec` feature enabled, it's | ||
/// recommended that your types have identical `TypeInfo` to those. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…or else? :)
Like, is "recommended" the right word here? Isn't it more "has to be"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, in theory it can be whatever it wants; it's just that our current tooling won't know how to decode the thing if the types don't line up with what we expect (no reason somebody else can't make tooling that can support more exotic types though if they want to go that route)
@@ -25,7 +25,7 @@ scale = { package = "parity-scale-codec", version = "3", default-features = fals | |||
[features] | |||
default = ["std"] | |||
std = [ | |||
"bitvec/std", | |||
"bitvec?/std", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that bitvec
will still ultimately be pulled in via scale/std
https://github.com/paritytech/parity-scale-codec/blob/b5f2b360e2939e8a235423f40591db09e879fdbb/Cargo.toml#L41
|
Requires Rust 1.60 for the optional dependency feature (? in Cargo.toml).
bitvec
in when the "std" feature is enabled, unless it's been specifically asked for. I don't believe that this is a breaking change, since while the bitvec crate would be included if just the "std" feature was enabled and isn't now, it looks like we hide that stuff behind an explicit "bit-vec" feature anyway, so hopefully it just avoids pulling in a crate we don't actually use in that case.TypeDefBitSequence::new()
to be called without thebit-vec
feature being enabled. This relaxes the constraints on it slightly, making "incorrect" uses more possible, but allowing us to create the type info without expecting any specific underlying library. Again, since this just relaxes/adds, it's not a breaking change.I also updated the UI test output just because it had changed a touch. There are a couple of unused lifetime warnings that can't be silenced but will be silencable in a future rust update (see rust-lang/rust#96956)
parity-scale-codec PR to help with this: paritytech/parity-scale-codec#375
Closes #167