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

Patch IsNumber decorator with maxDecimalPlaces option set for numbers in scientific annotation #2166

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

backnight
Copy link

@backnight backnight commented Jul 16, 2023

Description

The IsNumber() decorator doesn't work on scientifically annotated numbers when specifying maxDecimalPlaces option because value.toString() has no dot in it and crashes (see issue: #1705).

decimalPlaces = value.toString().split('.')[1].length;

This patch reworks the whole handling of maxDecimalPlaces option in order to make it work with scientific numbers.

The logic behind the patch is toFixed(maxDecimalPlaces) the input so it is represented as a string with a maximum number of decimals as specified by the option maxDecimalPlaces, then parseFloat() it to be compared with the original input.
If there is a difference with the original input, it means it has a number of decimals > maxDecimalPlaces because they have been truncated by toFixed(maxDecimalPlaces)

Checklist

  • the pull request title describes what this PR does (not a vague title like Update index.md)
  • the pull request targets the default branch of the repository (develop)
  • the code follows the established code style of the repository
    • npm run prettier:check passes
    • npm run lint:check passes
  • tests are added for the changes I made (if any source code was modified)
  • documentation added or updated
  • I have run the project locally and verified that there are no errors

Fixes

fixes #1705

The IsNumber() decorator doesn't work on scientifically annotated numbers when specifying maxDecimalPlaces options because value.toString() has no comma in it and crashes (line 36).

This patch replaces the call to toString() to toFixed() in order to transform the scientifically annotated number into a string with a comma, correctly representing the number and passing the checks.
@backnight backnight closed this Jul 16, 2023
Clear patch
@backnight backnight reopened this Jul 16, 2023
@backnight backnight changed the title Patch IsNumber.ts with maxDecimalPlaces option set for numbers in scientific annotation Patch IsNumber decorator with maxDecimalPlaces option set for numbers in scientific annotation Jul 16, 2023
Copy link

@skoniks skoniks left a comment

Choose a reason for hiding this comment

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

parseFloat(value.toFixed(...)) !== value works as expected
Waiting for approvement ✔

@maierthomas
Copy link

maierthomas commented May 8, 2024

also waiting... 😒

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

fix: IsNumber decorator fails on values ABS(x) < 0.000001 when using maxDecimalPlaces
3 participants