-
Notifications
You must be signed in to change notification settings - Fork 17
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
Adds Mixed Integer Ops #29
Conversation
/// # use ethnum::I256; | ||
/// use ethnum::U256; |
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.
nit: I would write this as:
/// # use ethnum::I256; | |
/// use ethnum::U256; | |
/// # use ethnum::{I256, U256}; |
And same for other occurrences 👇.
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.
My rationale is that it feels weird to only show one of the use
statements in the doc test but not the other.
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 majority of the methods only require importing Self, and that import is hidden. I tried to match the patterns of the context as closely as possible. I think since the docs will appear on the Self page, Self's use statement is implied, but Other's use statement is not. There is some precedent in std.
A trilema:
- Hide one use statement but not the other in the same doc test.
- Hide all use statements in one doc test but none in the other doc test.
- Hide/Show all use statements in all doc tests.
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.
Good point... In my mind there is actually a quadlema:
- Hide all
ethnum
use statements but not other use statements in the same doc test
Which, while completely arbitrary, kind of makes sense in the context of usage documentation from the ethnum
crate's perspective.
I will merge this as is, and think about it a bit more to see if I want to change it or not.
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 pushed a commit addressing this right after you merged.
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.
Thanks - your points were valid though, and I'm still a bit on the fence on whether only Self
use
statements should be hidden or not.
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.
Thanks for the contribution!
This branch implements methods analogous to those of the stable feature
mixed_integer_ops
.Closes #24.