-
Notifications
You must be signed in to change notification settings - Fork 12k
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
Migrate math tests to ethers.js v6 #4769
Conversation
|
test/helpers/iterate.js
Outdated
const max = (...values) => values.slice(1).reduce((x, y) => (x > y ? x : y), values.at(0)); | ||
const min = (...values) => values.slice(1).reduce((x, y) => (x < y ? x : y), values.at(0)); | ||
const sum = (...values) => values.slice(1).reduce((x, y) => x + y, values.at(0)); |
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.
What do you think about keeping this in math.js
and reexporting them here in ./iterate
?
Alternative I'd propose getting rid of math.js
, I just feel this is redundant.
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.
We could also put them in math.js, and remove them completelly (not reexport) from iterate.js.
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.
Yeah, let's put them in math.js makes sense to me. Also, just found out that this sum implementation returns undefined
for an empty array. Not sure if we want that
Co-authored-by: Ernesto García <[email protected]>
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.
LGTM. I consider the comments for both moving the iterate
functions to math
and putting testCommutative
in a single place are non-blocking 👍🏻
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.
LGTM, thanks! 💪🏻
PR Checklist
npx changeset add
)