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

[Progress] Fix to validate the sum of the percents before rounding #759

Closed
wants to merge 1 commit into from
Closed

Conversation

ryamaguchi0220
Copy link
Contributor

@ryamaguchi0220 ryamaguchi0220 commented May 21, 2019

Description

In addition to #758 , I think that we should validate the sum of the percents before rounding instead of after rounding, because, as the error message indicates, the validation is for total and value as entered by the user.
スクリーンショット 2019-05-21 12 26 46

Testcase

https://jsfiddle.net/e0hvd7xo/
https://jsfiddle.net/562t0f3z/

Closes

#757

@ryamaguchi0220 ryamaguchi0220 changed the title fix(progress): Fix to validate the sum of the percents before rounding [Progress] Fix to validate the sum of the percents before rounding May 21, 2019
@lubber-de
Copy link
Member

The problem is: The rounding issues are a general Javascript problem. If we don't round them before comparing (it's actually correction instead of rounding to avoid values like 100.000000003 because of multiplication and division with the same value at the same time) the comparison probably always fails

@lubber-de lubber-de added state/awaiting-reviews Pull requests which are waiting for reviews lang/javascript Anything involving JavaScript labels May 21, 2019
@lubber-de
Copy link
Member

lubber-de commented May 21, 2019

@ryamaguchi0220 Your PR does not fix the example of your given jsfiddle when the sum is definately exceeding the total value (325+1, 111, 74, 612 =1123, but total given is 1122) , which of course gives a percentage > 100% (regardless of the rounding)

Testcases with sum(values) > given total

All fixes break because the sum of all values definaltey exceed the given total sum regardless of any rounding order or precision calculation

BUT

Testcases with sum(values) = given total

... what your PR indeed does fix is some behavior with the precision setting of 10. This currently still breaks in #758

@exoego You both should join forces for this precision issue 😉

@lubber-de lubber-de added the type/bug Any issue which is a bug or PR which fixes a bug label May 21, 2019
@lubber-de lubber-de added this to the 2.7.6 milestone May 21, 2019
@ryamaguchi0220
Copy link
Contributor Author

ryamaguchi0220 commented May 21, 2019

@lubber-de Thank you for your review!

Your PR does not fix the example of your given jsfiddle

What I'd like to test in the jsfiddle(https://jsfiddle.net/562t0f3z/) is that if the sum of the value exceeds the total the error will occur, because I think it is right behavior that the error occurs when the user's input is invalid.
So it's as I expected.
What do you think?

@exoego
Copy link
Contributor

exoego commented May 21, 2019

Thanks for proposing this changes. 👍
Closing since the changes are merged into #758
Please give your feedback on #758 or feel free to reopen if you have something.

@exoego exoego closed this May 21, 2019
@exoego exoego added type/duplicate Anything which is a duplicate and removed state/awaiting-reviews Pull requests which are waiting for reviews labels May 21, 2019
@lubber-de
Copy link
Member

Because I think it is right behavior that the error occurs when the user's input is invalid.
So it is my expectation.
What do you think?

Yes, definately. The console error in that case is correct and does not need a change

@ryamaguchi0220 ryamaguchi0220 deleted the dev branch May 21, 2019 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang/javascript Anything involving JavaScript type/bug Any issue which is a bug or PR which fixes a bug type/duplicate Anything which is a duplicate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants