Skip to content

lib: refactor to use validateInteger#45772

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
deokjinkim:221208_use_min_max_validatenumber
Feb 3, 2023
Merged

lib: refactor to use validateInteger#45772
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
deokjinkim:221208_use_min_max_validatenumber

Conversation

@deokjinkim

@deokjinkim deokjinkim commented Dec 7, 2022

Copy link
Copy Markdown
Contributor

Use validateInteger for record and revise error code of percentile.

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Dec 7, 2022
@deokjinkim
deokjinkim force-pushed the 221208_use_min_max_validatenumber branch from 4d67a36 to 1bbbed3 Compare December 7, 2022 15:51
@deokjinkim deokjinkim changed the title histogram: refactor to use min/max of validateNumber lib: refactor to use min/max of validateNumber Dec 7, 2022
Fix NumberIsNaN is called two times and change error code
to ERR_OUT_OF_RANGE when RangeError is occurred in test.
@deokjinkim
deokjinkim force-pushed the 221208_use_min_max_validatenumber branch from 1bbbed3 to 89113b8 Compare December 7, 2022 15:57
Comment thread lib/internal/histogram.js Outdated

if (NumberIsNaN(percentile) || percentile <= 0 || percentile > 100)
throw new ERR_INVALID_ARG_VALUE.RangeError('percentile', percentile);
validateNumber(percentile, 'percentile', 1, 100);

@aduh95 aduh95 Dec 7, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This forbids all the numbers between 0 and 1 that were previously allowed.

@deokjinkim deokjinkim Dec 7, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My bad. I can't use min/max of validateNumber here because zero have not to be allowed for percentile. Instead, I changed error code to ERR_OUT_OF_RANGE. Plus, I find another place to use validateInteger. Please take a look when you are available.

@deokjinkim deokjinkim changed the title lib: refactor to use min/max of validateNumber lib: refactor to use validateInteger Dec 7, 2022
@deokjinkim deokjinkim changed the title lib: refactor to use validateInteger lib: refactor to use validateInteger Dec 7, 2022
Use validateInteger for record and revise error code of percentile.
@deokjinkim
deokjinkim force-pushed the 221208_use_min_max_validatenumber branch from 9d44414 to 1830caf Compare December 7, 2022 23:37
@aduh95 aduh95 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 8, 2022
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 18, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 18, 2023
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@lpinca

lpinca commented Feb 2, 2023

Copy link
Copy Markdown
Member

@nodejs/tsc this needs another approval.

@mcollina mcollina added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Feb 3, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 3, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 88d71dc into nodejs:main Feb 3, 2023
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 88d71dc

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

Labels

commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants