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

Rename the _() translation function #1812

Open
Athanasius opened this issue Jan 16, 2023 · 3 comments · Fixed by #2209
Open

Rename the _() translation function #1812

Athanasius opened this issue Jan 16, 2023 · 3 comments · Fixed by #2209
Assignees
Labels
enhancement Plugins Anything related to plugins python Pull requests that update Python code Staged Feature Complete and in Testing for Next Release Translations
Milestone

Comments

@Athanasius
Copy link
Contributor

An unfortunate side-effect of forcing _() into builtins for translation of strings is that it means the _ for things like "I don't care about this return value" clashes. In general it was a REALLY bad choice given the uses of _ normally.

        should_return: bool
        new_data: dict[str, Any]
        should_return, _ = killswitch.check_killswitch('capi.auth', {})

Here I've changed new_data in the function call to _. After this the use of _("string to be translated") breaks:

Traceback (most recent call last):
  File "C:\Program Files\Python311\Lib\tkinter\__init__.py", line 1948, in __call__
    return self.func(*args)
           ^^^^^^^^^^^^^^^^
  File "C:\Users\Athan\Documents\Devel\EDMarketConnector\EDMarketConnector.py", line 1128, in capi_request_data
    self.status['text'] = _('Fetching data...')
                          ^^^^^^^^^^^^^^^^^^^^^
TypeError: 'dict' object is not callable

Proposal:
Change the translation _() to _t() - it's only one extra character.

Issues:
minor - PLUGINS.md mentions manually setting this up (does that code even work ?), that needs updating. But as plugins would have to manually set things up in this manner nothing will break by changing this in core code.

Follow-ups:
Identify any places where _ as "throw this away" would be appropriate and update them. There's at least some killswitch related code.
Check over all the developer documentation to be sure it's up to date with the change.

@Athanasius Athanasius added enhancement Plugins Anything related to plugins Translations python Pull requests that update Python code labels Jan 16, 2023
@Athanasius Athanasius modified the milestones: 5.8.0, 5.9.0, Future Release with major code changes Jan 16, 2023
@Rixxan Rixxan removed this from the Future Release with major code changes milestone Aug 4, 2023
@Rixxan Rixxan added this to the 6.0.0 milestone Dec 21, 2023
@Rixxan Rixxan added this to Pending in Enhancement Tracker via automation Mar 23, 2024
@Rixxan
Copy link
Contributor

Rixxan commented Apr 4, 2024

Honestly, I'm looking at just dropping forcing the builtin entirely. It confuses type checking, it makes things look weird, and it's not very clear where it's set or what it's doing.

It'd be easier to just add a dummy function to the l10n _Translation class and accept one more direct import.

I don't think it's too much of a design sacrifice to do something along these lines:

Add this to l10n's _Translations class

    def tl(self, *args) -> str:
        """
        Dummy loader for the translate function. Used as shorthand.
        """
        return self.translate(*args)

and in files where we need translations

from l10n import Translations as Tr
...
def somefunc():
        foo = Tr.tl("My String Here")
...

instead of the traditional

def somefunc():
        foo = _("My String Here")

@Rixxan Rixxan added this to In Progress in General Code Cleanups Apr 4, 2024
@Rixxan Rixxan moved this from Pending to In Progress in Enhancement Tracker Apr 4, 2024
@Rixxan Rixxan self-assigned this Apr 4, 2024
@Rixxan
Copy link
Contributor

Rixxan commented Apr 4, 2024

Mocked up what would need to be done in https://github.com/HullSeals/EDMarketConnector/tree/enhancement/1812/rename_underscore_translate
Tasks:

  • Rename all usages of the function in the current code
  • Add deprecation note to existing builtin override (6.0 removal)
  • Add shorthand translation reference func and update imports
  • Update documentation on how to localize with plugins

Some of this may relate to #2188 and may need revisited if/when it gets merged in.

@Athanasius
Copy link
Contributor Author

As per comments in Discord, I'd take this opportunity to move to the standard ways of naming things in Python.

  1. Translations is currently the singleton. This should not be capitalised!
  2. This also means that Tr shouldn't be capitalised either.

So:

  1. Rename the _Translations class (note the leading underscore) to Translations.
  2. Rename the Translations singleton to translations.
  3. If doing import shortening tricks, then use/recommend something like from l10n import translations as tr, and end up with tr.tl(...) replacing _(...).

@Rixxan Rixxan linked a pull request May 2, 2024 that will close this issue
@Rixxan Rixxan modified the milestones: 6.0.0, 5.11.0 May 6, 2024
@Rixxan Rixxan added the Staged Feature Complete and in Testing for Next Release label May 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Plugins Anything related to plugins python Pull requests that update Python code Staged Feature Complete and in Testing for Next Release Translations
Projects
2 participants