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.

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

fix : issue #2430 isNumeric doesn't support scientific notation #2447

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ShreySinha02
Copy link

@ShreySinha02 ShreySinha02 commented Sep 8, 2024

#2430 fix(isNumeric): Add support for scientific notation

This PR addresses the issue described in issue #2430, where the isNumeric function did not support scientific notation. The regular expression used in isNumeric was updated to correctly validate numeric strings in scientific notation formats, such as '2e+21' and '-3.5E-8'. This change ensures that isNumeric can handle a wider range of numeric formats and provides more accurate validation.

References

Issue: validator.js issue isNumeric doesn't support scientific notation #2430

Checklist

  • PR contains only changes related; no stray files, etc.
  • README updated (where applicable)
  • Tests written (where applicable)
  • References provided in PR (where applicable)

Copy link

codecov bot commented Sep 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.96%. Comparing base (f81d857) to head (4e616a4).
Report is 8 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##            master    #2447      +/-   ##
===========================================
- Coverage   100.00%   99.96%   -0.04%     
===========================================
  Files          110      111       +1     
  Lines         2510     2518       +8     
  Branches       633      635       +2     
===========================================
+ Hits          2510     2517       +7     
- Partials         0        1       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pano9000
Copy link
Contributor

pano9000 commented Sep 9, 2024

IMHO, the scientifc notation should probably be an option.
If I use isNumeric in a everyday type of situation, I would expect it to not accept any types of letters, but just numbers.

if we merge this, then numbers like "2E+G", which an everyday user would not recognize as a "useful" number", would return "true", which could maybe lead to confusion.

so I would rather tend to say -> make this an opt-in option, so that people who really need this can use it, and keep the default behaviour as is.

I would like to here additional opions on this though, maybe @WikiRik @profnandaa ?

What we probably should do either way is add a line about JS turning those huge numbers into these scientific notation type of numbers into the README, to make people aware about this edge case

@WikiRik
Copy link
Member

WikiRik commented Sep 9, 2024

Yes, agree about having it an opt-in option. No need to change defaults and introduce a breaking change

@ShreySinha02
Copy link
Author

@pano9000 @WikiRik Thanks for the feedback, everyone! I agree with the idea of making scientific notation support an opt-in option to avoid breaking the existing behavior and to ensure clarity for everyday use cases.

so should I work on updating the PR to include this as an optional feature. I'll add an argument, something like { allowScientific: true }, which users can enable if they need to validate scientific notation.

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.

4 participants