Tick formatter does not support grouping with locale #8987
Conversation
|
Thanks, 👍 this seems reasonable. This needs a test (does not need an image test, just create a formatter with the right settings and check that the formater returns the expected string) and as it is a subtle API change (the same user code will now produce a different plot) it also needs a note in https://github.com/matplotlib/matplotlib/tree/master/doc/api/api_changes (see the README). |
|
As it's been 3 years, I went ahead and rebased this to fix the conflicts. I'm not sure how we can test this, as it appears locale-specific, and when I run |
|
Oops, I needed to set the locale, which is done in tests, so this should be possible. |
|
Locally, I found out that this didn't work, as setting |
|
With the previous nbconvert-failing tests restarted, this passes codecov now. |
When formatting and setting useLocale to True, the numbers greater than 1000 are not grouped as specified by the locale. 1000 should be 1,000 in certain locales. By setting the last argument to locale.format_string the locale dependent grouping is honored.
Once `exclude_lines` is set, the default exclusion is overridden, so we can't ignore coverage on specific lines. This adds the default rule back so that can be done again.
|
Rebased to fix conflicts. |
| tmp_form.create_dummy_axis() | ||
| tmp_form.set_bounds(0, 10) | ||
| tmp_form.set_locs([1, 2, 3]) | ||
| assert sep in tmp_form(1e9) |
There was a problem hiding this comment.
This is not the easiest test to follow, and seems to use a default locale? If the locale doesn't have the separator does this test anything?
There was a problem hiding this comment.
As it accesses the POSIX locale database, I wasn't sure if you would need to have the locale installed to have it work, so didn't want to hardcode anything specific. But note that the test framework does set the locale to en_US, so it's not an environment-specific default.
There was a problem hiding this comment.
I you feel it tests something real then I'm fine with it, but it just seems a little self-referential.
There was a problem hiding this comment.
True, a little self-referential, but it would catch if say, in the recent refactor that caused the conflicts, the locale setting was lost somewhere.
| @@ -7,6 +7,7 @@ omit = matplotlib/_version.py | |||
|
|
|||
| [report] | |||
| exclude_lines = | |||
| pragma: no cover | |||
There was a problem hiding this comment.
Yes, see #8987 (comment) and #8987 (comment)
When formatting and setting useLocale to True, the numbers greater than 1000 are not grouped as specified by the locale. 1000 should be 1,000 in certain locales. By setting the last argument to locale.format_string the locale dependent grouping is honored.
PR Summary
PR Checklist