Skip to content

Adds Mixed Integer Ops#29

Merged
nlordell merged 4 commits into
nlordell:mainfrom
NCGThompson:main
Oct 25, 2023
Merged

Adds Mixed Integer Ops#29
nlordell merged 4 commits into
nlordell:mainfrom
NCGThompson:main

Conversation

@NCGThompson

Copy link
Copy Markdown
Contributor

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

Closes #24.

Comment thread src/int/api.rs
Comment on lines +455 to +456
/// # use ethnum::I256;
/// use ethnum::U256;

@nlordell nlordell Oct 25, 2023

Copy link
Copy Markdown
Owner

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

@nlordell nlordell left a comment

Copy link
Copy Markdown
Owner

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
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