Skip to content

/ parity-common

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

Jump to bottom

Implement Num from num-traits #480

Merged
merged 1 commit into from Dec 25, 2020
Merged

Implement Num from num-traits #480

merged 1 commit into from Dec 25, 2020

Conversation

@SamWilsn
Copy link
Contributor

@SamWilsn SamWilsn commented Dec 17, 2020

Implementing the Num trait makes the U256 (and other) number types easier to work with, specifically with from_str_radix.
@ordian
ordian reviewed Dec 17, 2020
View changes
Copy link
Member

@ordian ordian left a comment

Thanks!

I usually don't like adding more features based on third-party libs, because it their breaking changes will be our breaking changes (semver). However I don't really mind in this case if you have a good use-case for it.
primitive-types/impls/num-traits/src/lib.rs Outdated Show resolved Hide resolved
primitive-types/impls/num-traits/src/lib.rs Outdated Show resolved Hide resolved
@SamWilsn
Copy link
Contributor Author

@SamWilsn SamWilsn commented Dec 18, 2020

I think, since this is gated behind features, you could do a num-traits-03 feature when version 0.3 is released.

I'll get the changes done tomorrow for the other comments.
@SamWilsn
Implement Num from num-traits
Verified
This commit was signed with the committer’s verified signature.
SamWilsn Sam Wilson
GPG key ID: DF735792EBD36915 Learn about signing commits
Loading status checks…
a044034
@SamWilsn SamWilsn force-pushed the SamWilsn:num-traits branch from 4e520df to a044034 Dec 21, 2020
@ordian
ordian approved these changes Dec 21, 2020
View changes
Copy link
Member

@ordian ordian left a comment

Looks good, thanks!

(a note for maintainers: this is a non-breaking change)
@ordian ordian requested a review from niklasad1 Dec 23, 2020
@niklasad1
niklasad1 reviewed Dec 23, 2020
View changes
primitive-types/impls/num-traits/src/lib.rs

//! num-traits support for uint.

#![cfg_attr(not(feature = "std"), no_std)]

This comment has been minimized.

Sign in to view
@niklasad1

niklasad1 Dec 23, 2020 •
edited
Member

I don't understand why this is needed?

This comment has been minimized.

Sign in to view
@SamWilsn

SamWilsn Dec 24, 2020
Author Contributor

Bit of a leftover from moving from_str_radix into the uint crate. I don't believe this crate needs it ever, but there is a feature that enables std in uint and num-traits.
@niklasad1
niklasad1 approved these changes Dec 23, 2020
View changes
Copy link
Member

@niklasad1 niklasad1 left a comment

LGTM
@ordian
ordian reviewed Dec 24, 2020
View changes
primitive-types/src/lib.rs
@@ -22,6 +22,7 @@ use fixed_hash::{construct_fixed_hash, impl_fixed_hash_conversions};
#[cfg(feature = "scale-info")]
use scale_info::TypeInfo;
use uint::{construct_uint, uint_full_mul_reg};
pub use uint::{FromStrRadixErr, FromStrRadixErrKind};

This comment has been minimized.

Sign in to view
@ordian

ordian Dec 24, 2020
Member

hmm, I overlooked this line, seems redundant
we could return uint::FromStrRadixErr in impls/num-traits instead of $crate::...

This comment has been minimized.

Sign in to view
@SamWilsn

SamWilsn Dec 24, 2020
Author Contributor

I'm not super familiar with the macro hygiene rules, but what if the downstream crate did a extern crate uint as floop?

This comment has been minimized.

Sign in to view
@ordian

ordian Dec 24, 2020
Member

yeah, fair point, we can do what impl/rlp does, but still this like seems redundant
@ordian ordian merged commit da4262d into paritytech:master Dec 25, 2020
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ordian
Copy link
Member

@ordian ordian commented Dec 25, 2020

I'll address the nits in a follow-up.
ordian added a commit that referenced this pull request Dec 25, 2020
@ordian
primitive-types: address nits of #480
Verified
This commit was signed with the committer’s verified signature.
ordian Andronik Ordian
GPG key ID: 5FF5BADD61F40001 Learn about signing commits
Loading status checks…
2641855
@ordian ordian mentioned this pull request Dec 25, 2020
Merged
niklasad1 pushed a commit that referenced this pull request Jan 1, 2021
@ordian
primitive-types: address nits of #480 (#485)
Verified
This commit was created on GitHub.com and signed with GitHub’s verified signature.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
36004c9 
* primitive-types: address nits of #480

* fix ethereum-types
ordian added a commit that referenced this pull request Jan 4, 2021
@ordian
Merge branch 'master' into dependabot/cargo/smallvec-1.6.0
Verified
This commit was signed with the committer’s verified signature.
ordian Andronik Ordian
GPG key ID: 5FF5BADD61F40001 Learn about signing commits
Loading status checks…
766d96c 
* master:
  Remove deprecated FromStr/TryFrom impls for Secret (#495)
  primitive-types: address nits of #480 (#485)
ordian added a commit that referenced this pull request Jan 4, 2021
@ordian
Merge branch 'master' into ao-prepare-for-releases
Verified
This commit was signed with the committer’s verified signature.
ordian Andronik Ordian
GPG key ID: 5FF5BADD61F40001 Learn about signing commits
Loading status checks…
eeff88a 
* master:
  Remove deprecated FromStr/TryFrom impls for Secret (#495)
  primitive-types: address nits of #480 (#485)
ordian added a commit that referenced this pull request Jan 4, 2021
@ordian
Merge branch 'master' into dependabot/cargo/rand-0.8.0
Verified
This commit was signed with the committer’s verified signature.
ordian Andronik Ordian
GPG key ID: 5FF5BADD61F40001 Learn about signing commits
Loading status checks…
cd53d65 
* master:
  Remove deprecated FromStr/TryFrom impls for Secret (#495)
  primitive-types: address nits of #480 (#485)
  build(deps): update impl-trait-for-tuples requirement (#490)
  fix: make from_str parse 0x-prefixed strings (#487)
  update some dev-dependencies (#493)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ordian ordian

@niklasad1 niklasad1

Assignees
No one assigned
Labels
A0-pleasereview
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
@SamWilsn @ordian @niklasad1