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.

Sign up for GitHub

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

Replace integer check with Number.isInteger #405

Merged
merged 1 commit into from
Mar 15, 2019
Merged

Replace integer check with Number.isInteger #405

merged 1 commit into from
Mar 15, 2019

Conversation

mAAdhaTTah
Copy link
Contributor

This works correctly for large numbers.

Related to #147

@mAAdhaTTah
Copy link
Contributor Author

I'm not sure what your policy is for using newer features. I can inline the Number.isInteger polyfill from MDN if that's preferred. Or I can update the docs to indicate this is required so the user makes sure it's supported or is polyfilled.

@mAAdhaTTah
Copy link
Contributor Author

Looking at the test failure, I'm not sure why that was considered invalid.

jquense
jquense reviewed Jan 14, 2019
View reviewed changes
src/number.js Outdated Show resolved Hide resolved
jquense
jquense requested changes Jan 14, 2019
View reviewed changes
Copy link
Owner

@jquense jquense left a comment

Choose a reason for hiding this comment

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

see note! thanks for the PR

@mAAdhaTTah
Replace integer check with Number.isInteger
aa73340 
This works correctly for large numbers.

Related to jquense#147
@mAAdhaTTah
Copy link
Contributor Author

Addressed the comment & resolved the conflict.

@mAAdhaTTah
Copy link
Contributor Author

@jquense Bumping this. I think I've addressed your issues. Let me know if you've got other changes.

@jquense jquense merged commit 1c18442 into jquense:master Mar 15, 2019
@jquense
Copy link
Owner

jquense commented Mar 15, 2019

thanks!

@mAAdhaTTah mAAdhaTTah deleted the bugfix/is-integer branch March 15, 2019 15:40
This was referenced Aug 1, 2019
Closed
Merged
@hugonasciutti hugonasciutti mentioned this pull request Oct 24, 2019
Closed
PierreJeanjacquot added a commit to iExecBlockchainComputing/iexec-sdk that referenced this pull request Oct 25, 2019
@PierreJeanjacquot
workarond yup integer fail (waiting jquense/yup/pull/405)
50fab67
@pleunv
Copy link
Contributor

pleunv commented Dec 12, 2019

Just discovered that Number.isInteger(Number.MAX_SAFE_INTEGER + 1) returns true, which means that this could cause issues in validation. There's also Number.isSafeInteger() which would invalidate anything higher than Number.MAX_SAFE_INTEGER. Should Yup be using this one instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jquense jquense jquense approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mAAdhaTTah @jquense @pleunv