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 parsing in locales that use a different decimal separator. #59

Closed
wants to merge 2 commits into from

Conversation

AbdielKavash
Copy link
Member

@AbdielKavash AbdielKavash commented Jan 26, 2024

This is a correction to #58.

Number parsing in MathExpression.parseMathExpression() will now try to use a NumberFormat given by the TextFieldWidget using it; or the system default NumberFormat if none has been provided (or if called from outside a TextFieldWidget).

This means that numbers are now correctly parsed in locales that use a different thousands and decimal separators than , and .. Hopefully this should fix tests failing for @Dream-Master .

However current code still uses a mix of locale-aware formatting, and String.valueOf(). Also, since the raw text of the widget is exposed to the user, there is no way to guarantee that whoever is using the widget follows locale settings correctly.

Specifically for integer numbers, right now, this does not matter; as BaseTextFieldWidget hides thousands separators; and so most common number formats will appear identical. But if anybody wants a widget that can show non-integer decimal numbers, or to show thousands separators (the latter of which I would like to do), this could cause problems.

(And just for clarity, this problem is not caused by either this PR or the previous one; it was present in the code before too. It was only not apparent because people didn't try to input decimal numbers into the text field.)

I can see at least two possible solutions:

  • Stick our head into the sand and mandate the US (or some other) locale on everybody. (I am not necessarily implying that this is unreasonable.)
  • Create a new type of widget, NumericWidget, which would only expose the numeric value to the user, and manage all input/parsing/display on its own. Then switch over any code that previously used a TextWidget to input numbers to the new numeric widget.

@Dream-Master
Copy link
Member

Dream-Master commented Jan 26, 2024

Now more processes passed onle one not pass

    org.opentest4j.AssertionFailedError: expected: <123456.0> but was: <123.456>
        at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
        at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
        at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
        at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:70)
        at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:65)
        at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:886)
        at com.gtnewhorizons.modularui.api.math.MathExpressionTest.NumbersBasic_Test(MathExpressionTest.java:21)

@Dream-Master Dream-Master requested a review from a team January 26, 2024 11:28
@AbdielKavash
Copy link
Member Author

The failing test case should now be fixed (issue was in the test, not in the code). Please let me know if anything else fails.

@Dream-Master
Copy link
Member

will try later

@AbdielKavash
Copy link
Member Author

This PR is deprecated, I have reworked the parser more thoroughly and in the process addressed these issues as well.

See #60.

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.

2 participants