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

Escape space after comma as decimal seperator #24079

Closed

Conversation

evanandresen
Copy link

@evanandresen evanandresen commented Oct 2, 2022

PR Summary

When creating a plot with a locale where a comma is used as the decimal seperator and with useMathText enabled for ticks, an unwanted space is added after the comma (Example: "3, 0"). This PR addresses this issue #23626 by adding an escape around the comma in a formatting method for the ticks.

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • [N/A] New features are documented, with examples if plot related.
  • [N/A] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [N/A] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • [N/A] Documentation is sphinx and numpydoc compliant (the docs should build without error).

formatted = self.fix_minus(locale.format_string(fmt, (arg,), True)
if self._useLocale else fmt % arg)
if (self.get_useMathText()): # removed unintended space after comma
formatted = formatted.replace(',', '{,}')
Copy link
Contributor

@anntzer anntzer Oct 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would incorrectly also escape commas actually present in fmt.

I guess a simple-ish solution could be to str.split fmt over commas, apply locale.format_string over each part, escape resulting commas over each formatted part, and join again using unescaped commas.

Edit: basically

        return self.fix_minus(
            # Escape commas introduced by format_string but not those present
            # from the beginning in fmt.
            ",".join(locale.format_string(part, (arg,), True)
                     .replace(",", "{,}")
                     for part in fmt.split(","))
            if self._useLocale else
            fmt % arg)

def test_locale_comma():
import locale
currentLocale = locale.getlocale()
locale.setlocale(locale.LC_ALL, 'deu_deu')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test will fail with "unsupported locale setting" if the locale is unavailable; at least this should be caught, and it may be better to just monkeypatch locale.format_string instead?

Copy link
Contributor

@anntzer anntzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

anyone can dismiss once comments are handled.

@jklymak
Copy link
Member

jklymak commented Jan 30, 2023

Moving to draft pending revision. @evanandresen anything we can do to help move this forward?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants