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

feat(isFloat) Fix , passing as float #2020

Closed
wants to merge 1 commit into from

Conversation

frederike-ramin
Copy link
Contributor

Similar to #752: Only passing , as string will result in a positive outcome of isFloat. This simply catches the case similar to . and + etc.

Checklist

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

@codecov
Copy link

codecov bot commented Aug 8, 2022

Codecov Report

Merging #2020 (8db790f) into master (86a07ba) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #2020   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          104       104           
  Lines         2203      2203           
  Branches       477       477           
=========================================
  Hits          2203      2203           
Impacted Files Coverage Δ
src/lib/isFloat.js 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@rubiin rubiin added the ✅ LGTM label Aug 9, 2022
@frederike-ramin
Copy link
Contributor Author

@rubiin do I still need to do anything to have this PR merged?

@rubiin
Copy link
Member

rubiin commented Oct 17, 2022

Waiting for further approvals from @profnandaa

@@ -5,7 +5,7 @@ export default function isFloat(str, options) {
assertString(str);
options = options || {};
const float = new RegExp(`^(?:[-+])?(?:[0-9]+)?(?:\\${options.locale ? decimal[options.locale] : '.'}[0-9]*)?(?:[eE][\\+\\-]?(?:[0-9]+))?$`);
Copy link
Contributor

@gibson042 gibson042 Oct 26, 2022

Choose a reason for hiding this comment

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

Suggested change
const float = new RegExp(`^(?:[-+])?(?:[0-9]+)?(?:\\${options.locale ? decimal[options.locale] : '.'}[0-9]*)?(?:[eE][\\+\\-]?(?:[0-9]+))?$`);
// largely derived from ECMAScript, with optional leading sign and allowing unnecessary leading zeroes
// but not underscore separators
// https://tc39.es/ecma262/multipage/ecmascript-language-lexical-grammar.html#prod-DecimalLiteral
const float = new RegExp(`^[-+]?(?:[0-9]+|(?=.[0-9]))(?:\\${options.locale ? decimal[options.locale] : '.'}[0-9]*)?(?:[eE][-+]?[0-9]+)?$`);

(and if you take this suggestion, then the following if is unnecessary)

@profnandaa profnandaa added mc-to-land Just merge-conflict standing between the PR and landing. and removed ready-to-land For PRs that are reviewed and ready to be landed labels Feb 1, 2023
@profnandaa
Copy link
Member

merging in #2164

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎉 first-pr mc-to-land Just merge-conflict standing between the PR and landing. PR/combined
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants