You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2020/03/15 12:03:31 UTC
[GitHub] [logging-log4j2] trejkaz opened a new pull request #349: Fix
erroneous usage of default locale
trejkaz opened a new pull request #349: Fix erroneous usage of default locale
URL: https://github.com/apache/logging-log4j2/pull/349
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [logging-log4j2] remkop commented on issue #349: Fix erroneous
usage of default locale
Posted by GitBox <gi...@apache.org>.
remkop commented on issue #349: Fix erroneous usage of default locale
URL: https://github.com/apache/logging-log4j2/pull/349#issuecomment-599269469
That makes sense. Thank you for the clarification.
(Perhaps in that case a test could be created with `Locale.setDefault` where the old code has a problem, that goes away with your fix. Just remember to reset the default locale to the original value after the test completed. )
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [logging-log4j2] remkop commented on issue #349: Fix erroneous
usage of default locale
Posted by GitBox <gi...@apache.org>.
remkop commented on issue #349: Fix erroneous usage of default locale
URL: https://github.com/apache/logging-log4j2/pull/349#issuecomment-599266708
If providing a test is challenging, can you at least provide a rationale on why the change is necessary? Why is Locale.ROOT better than Locale.getDefault()? What problem are you trying to solve?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [logging-log4j2] trejkaz commented on issue #349: Fix erroneous
usage of default locale
Posted by GitBox <gi...@apache.org>.
trejkaz commented on issue #349: Fix erroneous usage of default locale
URL: https://github.com/apache/logging-log4j2/pull/349#issuecomment-599873519
Alright, so as it turns out, all current locales show no failure with passing in the digits in Latin even if that locale in question uses different digits, which must be a hidden feature of NumberFormat's parser.
Although:
There is _definitely_ a way I can make this code break, it involves substituting the LocaleServiceProvider / NumberFormatProvider, which is how I discovered this issue in the first place, but that's a whole other topic which I promise to you will look a lot more contrived if I contributed that as the test case.
So at this point it is far easier to just say: using the default locale in library code should be avoided - I'm removing one such usage of the code free of charge. :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [logging-log4j2] trejkaz commented on issue #349: Fix erroneous
usage of default locale
Posted by GitBox <gi...@apache.org>.
trejkaz commented on issue #349: Fix erroneous usage of default locale
URL: https://github.com/apache/logging-log4j2/pull/349#issuecomment-599266980
A config file should be assumed to work the same irrespective of the locale the machine is running in. By parsing numbers in the default locale there is the risk of the behaviour being different for someone running in a different number system.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [logging-log4j2] remkop commented on issue #349: Fix erroneous
usage of default locale
Posted by GitBox <gi...@apache.org>.
remkop commented on issue #349: Fix erroneous usage of default locale
URL: https://github.com/apache/logging-log4j2/pull/349#issuecomment-599878124
I see. I was thinking along the lines of parsing `1,000` (1000 in US locale) vs `1.000` (1000 in French/German/Dutch locale), but if that doesn't work for testing then I don't object to your proposed change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [logging-log4j2] trejkaz commented on issue #349: Fix erroneous
usage of default locale
Posted by GitBox <gi...@apache.org>.
trejkaz commented on issue #349: Fix erroneous usage of default locale
URL: https://github.com/apache/logging-log4j2/pull/349#issuecomment-599222520
That seems challenging.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [logging-log4j2] remkop commented on issue #349: Fix erroneous
usage of default locale
Posted by GitBox <gi...@apache.org>.
remkop commented on issue #349: Fix erroneous usage of default locale
URL: https://github.com/apache/logging-log4j2/pull/349#issuecomment-599919068
Thanks for adding the test. This looks good to merge.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [logging-log4j2] trejkaz commented on issue #349: Fix erroneous
usage of default locale
Posted by GitBox <gi...@apache.org>.
trejkaz commented on issue #349: Fix erroneous usage of default locale
URL: https://github.com/apache/logging-log4j2/pull/349#issuecomment-599265206
I thought I'd try introducing forbidden-apis, but while looking at the logs, I notice that error-prone is in use already, so it's quite possible that this change has reduced the warning count by 1. It doesn't look like GitHub's CI thingy notices that sort of thing though.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [logging-log4j2] rgoers merged pull request #349: Fix erroneous
usage of default locale
Posted by GitBox <gi...@apache.org>.
rgoers merged pull request #349: Fix erroneous usage of default locale
URL: https://github.com/apache/logging-log4j2/pull/349
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [logging-log4j2] remkop commented on issue #349: Fix erroneous
usage of default locale
Posted by GitBox <gi...@apache.org>.
remkop commented on issue #349: Fix erroneous usage of default locale
URL: https://github.com/apache/logging-log4j2/pull/349#issuecomment-599203151
Could you add a breaking test that passes with your patch?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [logging-log4j2] trejkaz commented on issue #349: Fix erroneous
usage of default locale
Posted by GitBox <gi...@apache.org>.
trejkaz commented on issue #349: Fix erroneous usage of default locale
URL: https://github.com/apache/logging-log4j2/pull/349#issuecomment-599881590
Ah, you're right, that case does break it because it turns into a decimal point.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services