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 incorrect calculation of RoundUp/RoundDown because value after rescaling was ignored #202

Merged
merged 1 commit into from Jan 17, 2021

Conversation

lovung
Copy link
Contributor

@lovung lovung commented Jan 15, 2021

Why

The current implementation uses the rescale method before deciding to Add/Sub 1 for the rescaled value.
But, it will be wrong in the case the rescaled value equals the original value.

Fix

Compare the value after rescaling.

@mwoss
Copy link
Member

mwoss commented Jan 16, 2021

Ups, I think I forgot about this case. Thank you for the fix @lovung !
Could you also add a test case to cover that case? :DD

@lovung
Copy link
Contributor Author

lovung commented Jan 16, 2021

@mwoss I have already added 4 test cases. Do you want me to add more? Because I think 4 are enough.

@mwoss
Copy link
Member

mwoss commented Jan 17, 2021

I didn't notice those test cases yesterday, sorry. :D
Looks good to me :3 One more thing only, could you add a description to this PR with an explanation of what exactly this PR is solving? It would be helpful for potential future code modifications.

Copy link
Member

@mwoss mwoss left a comment

Choose a reason for hiding this comment

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

Looks good :3
Only one small request about updating PR description :DD

@lovung
Copy link
Contributor Author

lovung commented Jan 17, 2021

@mwoss I have updated the description. Could you take a look? Thank you so much.

@mwoss mwoss changed the title Fix wrong return RoundUp/RoundDown because of value after rescaled is ignored Fix incorrect calculation of RoundUp/RoundDown because value after rescaling was ignored Jan 17, 2021
@mwoss mwoss merged commit 99f4d74 into shopspring:master Jan 17, 2021
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.

None yet

2 participants