Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Add ensure_pow method #13042

Merged
merged 3 commits into from
Jan 9, 2023
Merged

Conversation

lemunozm
Copy link
Contributor

@lemunozm lemunozm commented Jan 2, 2023

This PR extends #12967 with the ensure_pow function. I think this is also an interesting function to have along with the ensure_ops family methods.

ensure_pow acts similarly to checked_pow but returns an ArithmeticError instead.

Related #12754

@lemunozm lemunozm changed the title add ensure_pow method Add ensure_pow method Jan 2, 2023
@bkchr bkchr requested a review from ggwpez January 2, 2023 23:09
@bkchr bkchr added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit A0-please_review Pull request needs code review. labels Jan 2, 2023
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some minimal success tests or merge in #13043 and use the test structure from there.

primitives/arithmetic/src/traits.rs Outdated Show resolved Hide resolved
@lemunozm
Copy link
Contributor Author

lemunozm commented Jan 3, 2023

I've implemented ensure_pow as a function because checked_pow is also implemented as a function. But maybe can be interesting to follow the same pattern as other ensure_ops and make a trait EnsurePow with an ensure_pow method. What do you think? I'm not sure why in num-traits the checked_pow is not implemented as a trait TBH.

@ggwpez
Copy link
Member

ggwpez commented Jan 4, 2023

I've implemented ensure_pow as a function because checked_pow is also implemented as a function. But maybe can be interesting to follow the same pattern as other ensure_ops and make a trait EnsurePow with an ensure_pow method. What do you think? I'm not sure why in num-traits the checked_pow is not implemented as a trait TBH.

Dont know what is better here. Maybe there can be types that are not CheckedMul but can still have an ensure_pow?
I cant imagine but that would not work with a free-standing function otherwise.

@lemunozm
Copy link
Contributor Author

lemunozm commented Jan 9, 2023

Added tests. I think all pending comments are "solved" for now.

I've opened an issue rust-num/num-traits#254 trying to get more info in order to know the best possible design decision for doing it as a function or as a trait method. But I think is not blocking, so if you are ok with it right now, it's ok for me.

@lemunozm lemunozm requested review from bkchr and ggwpez and removed request for bkchr and ggwpez January 9, 2023 17:22
@ggwpez
Copy link
Member

ggwpez commented Jan 9, 2023

(Looks like the CI does not notice when you dismiss reviews 🤔)

Merging now since there were no changes since Basti last reviewed.

@ggwpez
Copy link
Member

ggwpez commented Jan 9, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit b85246b into paritytech:master Jan 9, 2023
@lemunozm lemunozm deleted the lm-ensure-pow branch January 9, 2023 21:05
@lemunozm
Copy link
Contributor Author

lemunozm commented Jan 9, 2023

(Looks like the CI does not notice when you dismiss reviews 🤔)

Yes, it was weird, the review button switched between you and your partner, instead of selecting both 🤷‍♂️.

Anyways, thanks for your reviews! 🙌🏻

rossbulat pushed a commit that referenced this pull request Jan 11, 2023
* add ensure_pow method

* reexport checked_pow and ensure_pow
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-release-analysis-v0-9-37/1736/1

ltfschoen pushed a commit to ltfschoen/substrate that referenced this pull request Feb 22, 2023
* add ensure_pow method

* reexport checked_pow and ensure_pow
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* add ensure_pow method

* reexport checked_pow and ensure_pow
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants