-
Notifications
You must be signed in to change notification settings - Fork 158
Option-like API for variant #383
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
Conversation
Interesting, what's the intended use for that? |
src/generate/generic.rs
Outdated
pub fn unwrap(self) -> T { | ||
match self { | ||
Val(v) => v, | ||
Res(u) => panic!("Unexpected variant: {:?}", u), |
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'm not to hot about this panic!
and the expect
method below. If this ends up in use in real code it'd add significant bloat.
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 can remove unwrap
and keep expect
as it is more explicit.
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'm fine with with those methods per se but let's get rid of the formatting and the Debug
bound, please.
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.
Ok
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.
Is now better?
This comment has been minimized.
This comment has been minimized.
src/generate/generic.rs
Outdated
use Variant::*; | ||
impl<U, T> Variant<U, T> { | ||
/// Check if the variant is expected | ||
pub fn is_value(&self) -> bool { |
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.
This naming is strange. At least function name does not correspond to the docs. I can't suggest anything nicer, though.
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.
Do you have suggestions?
/// | ||
/// Panics if the value is an `Res`, with a panic message including the | ||
/// passed message, and the content of the `Res`. | ||
pub fn expect(self, msg: &'static str) -> T { |
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.
Do you really need 'static
bound here?
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 doesn’t compile without 'static
.
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.
Weird. The original expect doesn't have it.
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 think it's because original expect
use formatted panic!("{}", msg)
.
With 2 arguments.
Rebased. |
See #496 |
r? @therealprof
cc @Disasm