-
Notifications
You must be signed in to change notification settings - Fork 223
Perform locale-sensitive string comparison in BooleanConverter
#367
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @basil,
Thanks for the PR! 💯 It looks good to me, but I added some remarks below.
For context (and to anticipate possible feedback from other reviewers): the current approach of using toLowerCase(Locale.ROOT)
to compare custom true/false strings doesn’t work well in an internationalized context.
On the other hand, allowing custom strings only in English doesn’t seem very useful, since the default values already cover nearly all meaningful alternatives for “true” and “false.”
Given that, I’d suggest we either:
- Accept this PR, which correctly handles case-insensitive and locale-dependent Unicode comparisons, or
- Remove the ability to supply custom strings to
BooleanConverter
altogether.
src/main/java/org/apache/commons/beanutils2/converters/BooleanConverter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/commons/beanutils2/converters/BooleanConverter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/commons/beanutils2/converters/BooleanConverter.java
Outdated
Show resolved
Hide resolved
src/test/java/org/apache/commons/beanutils2/converters/BooleanConverterTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/apache/commons/beanutils2/converters/BooleanConverterTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/apache/commons/beanutils2/converters/BooleanConverterTest.java
Outdated
Show resolved
Hide resolved
…Converter.java Co-authored-by: Piotr P. Karwasz <[email protected]>
…Converter.java Co-authored-by: Piotr P. Karwasz <[email protected]>
…ConverterTest.java Co-authored-by: Piotr P. Karwasz <[email protected]>
…ConverterTest.java Co-authored-by: Piotr P. Karwasz <[email protected]>
…ConverterTest.java Co-authored-by: Piotr P. Karwasz <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but let's wait until the discussion on dev@commons
is over before merging.
Extends #366 (so is not subject to @garydgregory's veto, which applied to a different hunk of code), but also incorporates a suggested enhancement from @ppkarwasz (adding full locale support to custom boolean types). The new test fails before the changes to
src/main
to do locale-sensitive string comparison and passes after them.mvn
; that'smvn
on the command line by itself.