-
Notifications
You must be signed in to change notification settings - Fork 18
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
impl TryFrom<T::Type> for BitFlags<T> #14
Conversation
fe5149c
to
1e95218
Compare
Otherwise, `T: Debug` isn't enough to imply `BitFlags<T>: Debug`, and this can get in the way of code generated by #[derive(Debug)].
enumflags/src/lib.rs
Outdated
}) | ||
} | ||
} | ||
|
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 don't think these should be also exposed as a method. A trait would be enough. In fact, I'm considering #[deprecating] from_bits
once this lands.
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.
Huh, I disagree with that philosophy in that I find generic conversion traits often unwieldy to use explicitly when you need them, because type inference often needs help via extra type hints/ascriptions when using something(BitFlags::from)
- whereas BitFlags::from_bits
assists inference and improves readability. I guess I'm not personally a fan of using conversion traits outside of generic arguments when there's a better explicit option (usually that's ::new
I guess).
pub fn invalid_bits(self) -> T::Type { | ||
self.invalid | ||
} | ||
} |
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.
Since this is a public API, it'd be nice to have a sentence or two of documentation here. You could even write a doctest which tries to convert a number with some invalid flags and then asserts about the results of truncate
and invalid_bits
. This would kill two birds with one stone, since there aren't really any tests for this functionality right now.
enumflags/src/fallible.rs
Outdated
impl_try_from! { $ty } | ||
impl_try_from! { $($tt)* } | ||
}; | ||
($ty:ty) => { |
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.
It's not necessary to use 3 rules here. I've pushed a commit onto your branch that shows how this can be done, though ideally it would be squashed into this one.
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.
Makes sense, will make sure to get it all get squashed/cleaned up once it's ready.
Misc notes:
std
flag because althoughTryFrom
doesn't have atype Error: std::error::Error
bound, it's often useful to have that impl. I'm wary of making it default though, because it still seems ideal to keep the core crateno_std
by default?default-features = false
)BitFlags<Self>
might work but that's not good enough for the orphan rules it seems.BitFlagNum
modified to includefmt::Debug
constraint because we kind of want#[derive(Debug)] struct S<T>(BitFlags<T>)
to work?BitFlags<T>
in context though... We hideBitFlagsRaw
as an internal trait despite the fact that it's required forBitFlags<T>
and thus pretty crucial for doing any generic operations over bitflags... I'm questioning whether we shouldn't re-export this publicly at the root of the crate? (if we don't want manual impls and want to still require use of the proc macro, we can still make the trait fns themselvesdoc(hidden)
)BitFlags<T>
vsT
that I'd like to address I think... Hm.