Skip to content
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

Merged
merged 4 commits into from
Oct 25, 2023
Merged

Adds Mixed Integer Ops #29

merged 4 commits into from
Oct 25, 2023

Conversation

NCGThompson
Copy link
Contributor

This branch implements methods analogous to those of the stable feature mixed_integer_ops.

Closes #24.

Comment on lines +455 to +456
/// # use ethnum::I256;
/// use ethnum::U256;
Copy link
Owner

@nlordell nlordell Oct 25, 2023

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:

Suggested change
/// # use ethnum::I256;
/// use ethnum::U256;
/// # use ethnum::{I256, U256};

And same for other occurrences 👇.

Copy link
Owner

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.

Copy link
Contributor Author

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.

Copy link
Owner

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.

Copy link
Contributor Author

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.

Copy link
Owner

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.

Copy link
Owner

@nlordell nlordell left a 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!

@nlordell nlordell merged commit 811cec8 into nlordell:main Oct 25, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement *_{add,sub}_{signed,unsigned} methods.
2 participants