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

fix: datatype.number when min = max + precision, throw when max > min #664

Merged
merged 15 commits into from
Mar 31, 2022
Merged

fix: datatype.number when min = max + precision, throw when max > min #664

merged 15 commits into from
Mar 31, 2022

Conversation

pkuczynski
Copy link
Member

Fixes #329

@pkuczynski pkuczynski requested a review from a team as a code owner March 23, 2022 21:15
@pkuczynski pkuczynski mentioned this pull request Mar 23, 2022
Closed
ST-DDT
ST-DDT reviewed Mar 24, 2022
View reviewed changes
src/datatype.ts Show resolved Hide resolved
src/datatype.ts Outdated Show resolved Hide resolved
@ST-DDT ST-DDT added c: bug Something isn't working p: 1-normal Nothing urgent labels Mar 24, 2022
@ST-DDT ST-DDT added this to the v6.1 - First bugfixes milestone Mar 24, 2022
@import-brain import-brain added the s: accepted Accepted feature / Confirmed bug label Mar 24, 2022
Shinigami92
Shinigami92 requested changes Mar 24, 2022
View reviewed changes
src/datatype.ts Outdated Show resolved Hide resolved
src/datatype.ts Outdated Show resolved Hide resolved
ST-DDT
ST-DDT previously approved these changes Mar 29, 2022
View reviewed changes
@ST-DDT ST-DDT requested a review from Shinigami92 March 29, 2022 20:43
@ST-DDT
Copy link
Member

ST-DDT commented Mar 29, 2022

Please fix the failing tests.

@ST-DDT ST-DDT removed the request for review from Shinigami92 March 29, 2022 20:43
@pkuczynski
Copy link
Member Author

Please fix the failing tests.

Yeah, I was working on them :)

@codecov
Copy link

codecov bot commented Mar 29, 2022 •
edited
Loading

Codecov Report

Merging #664 (007cb42) into main (0b350f9) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #664   +/-   ##
=======================================
  Coverage   99.34%   99.34%           
=======================================
  Files        1924     1924           
  Lines      177016   177010    -6     
  Branches      904      908    +4     
=======================================
- Hits       175863   175858    -5     
+ Misses       1097     1096    -1     
  Partials       56       56
Impacted Files Coverage Δ
src/address.ts 99.10% <100.00%> (ø)
src/datatype.ts 100.00% <100.00%> (ø)
src/random.ts 99.46% <100.00%> (+<0.01%) ⬆️
src/name.ts 100.00% <0.00%> (+0.28%) ⬆️

@pkuczynski
Copy link
Member Author

Please fix the failing tests.

All fixed now...

ST-DDT
ST-DDT reviewed Mar 29, 2022
View reviewed changes
src/address.ts Outdated Show resolved Hide resolved
test/address.spec.ts Show resolved Hide resolved
src/locales/ro/address/index.ts Outdated Show resolved Hide resolved
pkuczynski and others added 2 commits March 30, 2022 01:17
@pkuczynski
Update index.ts
9c312fc
@pkuczynski @ST-DDT
Update src/address.ts
ee6382e 
Co-authored-by: ST-DDT <ST-DDT@gmx.de>
@pkuczynski pkuczynski requested a review from ST-DDT March 29, 2022 23:19
Shinigami92
Shinigami92 previously approved these changes Mar 31, 2022
View reviewed changes
@ST-DDT
Copy link
Member

ST-DDT commented Mar 31, 2022 •
edited
Loading

Since my expectation of generateMeARandomNumberBetween(-5, 5) is to work, I will not approve this.
But I will accept it, if the others want it this PR's way.

My requirements:

  • Add a hint to the next changelog/release
  • Please also rename the PR to reflect the changes to behavior of the method (e.g. be more strict with datatype.number()'s behavior).

@Shinigami92
Copy link
Member

Since my expectation of generateMeARandomNumberBetween(-5, 5) is to work, I will not approve this. [...]

But I do understand it correctly that this PR did not introduce a change in that? And the test was just calling it wrongly in history?

ST-DDT
ST-DDT reviewed Mar 31, 2022
View reviewed changes
src/random.ts Show resolved Hide resolved
@pkuczynski
Copy link
Member Author

Since my expectation of generateMeARandomNumberBetween(-5, 5) is to work, I will not approve this. [...]

But I do understand it correctly that this PR did not introduce a change in that? And the test was just calling it wrongly in history?

This PR introduced an additional check if max > min and throws an error when they are not. We don't have a function that takes params like (-5, 5). It takes options ({ min: -5, max: 5 }). To see the error being thrown user would have to call it with ({ min: 5, max: -5 }) which is mathematically incorrect, right? So if anything we are only giving users better support now...

@pkuczynski pkuczynski changed the title fix: datatype.number does not produce random number when min = max + precision fix: datatype.number does not produce random number when min = max + precision, make the method more strict checking max > min Mar 31, 2022
@pkuczynski
Copy link
Member Author

My requirements:

  • Add a hint to the next changelog/release

Not sure how shall I do that?

  • Please also rename the PR to reflect the changes to behavior of the method (e.g. be more strict with datatype.number()'s behavior).

Done. Additionaly added some jsdoc.

@pkuczynski pkuczynski requested review from Shinigami92, ST-DDT and a team March 31, 2022 09:37
@Shinigami92
Copy link
Member

@pkuczynski Could you please shorten the title of this PR to something that is max 72 chars long?
The title is taken as commit title on squash-merge

@pkuczynski pkuczynski changed the title fix: datatype.number does not produce random number when min = max + precision, make the method more strict checking max > min fix: datatype.number when min = max + precision, throw when max > min Mar 31, 2022
@pkuczynski
Copy link
Member Author

@pkuczynski Could you please shorten the title of this PR to something that is max 72 chars long?
The title is taken as commit title on squash-merge

Done

ST-DDT
ST-DDT approved these changes Mar 31, 2022
View reviewed changes
Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

I changed my mind. number isn't the offender, latitude is.
So all is good.

@ST-DDT ST-DDT enabled auto-merge (squash) March 31, 2022 17:38
@ST-DDT ST-DDT merged commit 0304120 into faker-js:main Mar 31, 2022
@pkuczynski pkuczynski deleted the random-number-fix branch March 31, 2022 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Shinigami92 Shinigami92 Shinigami92 approved these changes

@ST-DDT ST-DDT ST-DDT approved these changes

Assignees

@pkuczynski pkuczynski

Labels
c: bug Something isn't working p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
None yet
Milestone
v6.1 - First bugfixes
Development

Successfully merging this pull request may close these issues.

datatype.number does not produce random number when min = max + precision
4 participants
@pkuczynski @ST-DDT @Shinigami92 @import-brain