EDCD / EDMarketConnector

Downloads commodity market and other station data from the game Elite: Dangerous for use with all popular online and offline trading tools.
GNU General Public License v2.0
989 stars 155 forks source link

Rename the `_()` translation function #1812

Closed Athanasius closed 4 months ago

Athanasius commented 1 year ago

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.

Rixxan commented 6 months ago

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 commented 6 months ago

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

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

Athanasius commented 6 months ago

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 _(...).