Some tests do not work on Locales using "," as decimal separator.

[reckart]

Bugfix change to get tests running on my non-US environment. Please consider this change to public domain.

[AngledLuffa]

I can't help but feel that this isn't the right answer. Wouldn't it make
more sense for the test to use the local Locale so that people get output
in the format they expect?

John

On Mon, Jan 20, 2014 at 1:56 PM, Richard Eckart de Castilho <
[email protected]> wrote:

Bugfix change to get tests running on my non-US environment. Please

consider this change to public domain.

You can merge this Pull Request by running

git pull https://github.com/reckart/CoreNLP feature/localefix

Or view, comment on, or merge it at:

#14
Commit Summary

• Some tests do not work on Locales using "," as decimal separator.
• Some tests do not work on Locales using "," as decimal separator.
  Forcing Locale.US on formatting.
• Revert whitespace changes.

File Changes

• M src/edu/stanford/nlp/stats/Counters.javahttps://github.com/ Some tests do not work on Locales using "," as decimal separator. #14/files#diff-0(13)
• M src/edu/stanford/nlp/util/ConfusionMatrix.javahttps://github.com/ Some tests do not work on Locales using "," as decimal separator. #14/files#diff-1(7)

Patch Links:

• https://github.com/stanfordnlp/CoreNLP/pull/14.patch
• https://github.com/stanfordnlp/CoreNLP/pull/14.diff

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/14
.

[AngledLuffa]

For example, I just updated the ConfusionMatrix and its test so it will
hopefully work in your locale. A nice TODO would be to add expected output
for non-US Locales.

John

On Mon, Jan 20, 2014 at 8:03 PM, John Bauer [email protected] wrote:

I can't help but feel that this isn't the right answer. Wouldn't it make
more sense for the test to use the local Locale so that people get output
in the format they expect?

John

On Mon, Jan 20, 2014 at 1:56 PM, Richard Eckart de Castilho <
[email protected]> wrote:

Bugfix change to get tests running on my non-US environment. Please

consider this change to public domain.

You can merge this Pull Request by running

git pull https://github.com/reckart/CoreNLP feature/localefix

Or view, comment on, or merge it at:

#14
Commit Summary

• Some tests do not work on Locales using "," as decimal separator.
• Some tests do not work on Locales using "," as decimal separator.
  Forcing Locale.US on formatting.
• Revert whitespace changes.

File Changes

• M src/edu/stanford/nlp/stats/Counters.javahttps://github.com/ Some tests do not work on Locales using "," as decimal separator. #14/files#diff-0(13)
• M src/edu/stanford/nlp/util/ConfusionMatrix.javahttps://github.com/ Some tests do not work on Locales using "," as decimal separator. #14/files#diff-1(7)

Patch Links:

• https://github.com/stanfordnlp/CoreNLP/pull/14.patch
• https://github.com/stanfordnlp/CoreNLP/pull/14.diff

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/14
.

[reckart]

Sure, there are these two ways to follow. I believe creating the expected output in a locale-sensitive way ends up being much more work in the long run. E.g. the expected output cannot easily be stored in a file. It must always be generated dynamically. So, unless there is a specific requirement to support internationalization, I tend to fix the locale. Isn't it strange too when the messages and errors are in English, but the numeric formats are localized?

[reckart]

So I'm closing this pull request too.

I wonder, you do not seem to be merging the pull requests. It appears you just look at the changes and redo them. Otherwise, I believe the commits that are part of the pull request would appear as part of the project history. Is there a specific reason for this? E.g. missing CLA or pull requests with too many changes? Either can be fixed/improved.

[AngledLuffa]

Good observation. This is not the git repository we use internally, but a
mirror of the public code from a larger repository. It seemed like the
easiest way to make it so our experimental code was private and released
code was available. It also makes it easier to redo small changes rather
than pull twice.

I'm sure with a larger change such as the possible segmenter change, we'd
figure out how to pull from the public mirror,

John
On Jan 21, 2014 3:41 AM, "Richard Eckart de Castilho" <
[email protected]> wrote:

So I'm closing this pull request too.

I wonder, you do not seem to be merging the pull requests. It appears you
just look at the changes and redo them. Otherwise, I believe the commits
that are part of the pull request would appear as part of the project
history. Is there a specific reason for this? E.g. missing CLA or pull
requests with too many changes? Either can be fixed/improved.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/14#issuecomment-32856544
.

[AngledLuffa]

The test output doesn't have to be dynamically generated if you test the
results for a specific Locale, though, right? For example, the
ConfusionMatrix change I just submitted tests for US. It seems like
CountersTest could be set to test for US as well by calling
Locale.setDefault at the start of the test.

Would you check to see if the tests now pass? Thanks!

On Tue, Jan 21, 2014 at 3:37 AM, Richard Eckart de Castilho <
[email protected]> wrote:

Sure, there are these two ways to follow. I believe creating the expected
output in a locale-sensitive way ends up being much more work in the long
run. E.g. the expected output cannot easily be stored in a file. It must
always be generated dynamically. So, unless there is a specific requirement
to support internationalization, I tend to fix the locale. Isn't it strange
too when the messages and errors are in English, but the numeric formats
are localized?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/14#issuecomment-32849905
.

[reckart]

It appears to pass. Setting Locale.setDefault() sounds like a very good solution because it doesn't actually require changes to the API.