placeholder_delimiter config is ignored

[mbideau]

Hi @danhper and @ZRunner,

Thank you both (and other contributors) for this great piece of software.

I am facing an issue when attempting to change the placeholder delimiter (because I have data containing the default placeholder delimiter %)

Python 3.7.3 (default, Jan 22 2021, 20:04:44) 
[GCC 8.3.0] on linux
>>> import i18n
>>> i18n.set('error_on_missing_placeholder', True)
>>> i18n.set('placeholder_delimiter', '#')
>>> i18n.add_translation('hash', 'Hash: 10 # !')
>>> i18n.t('hash') # should raise the exception because it is using an invalid placeholder definition
'Hash: 10 # !'
>>> i18n.add_translation('perct', 'Percent: 10 % !')
>>> i18n.t('perct') # this one should not since I have changed the default delimiter
'Percent: 10 % !'
>>> i18n.t('perct')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/opt/collectd-reporting/lib/python3.7/site-packages/i18n/translator.py", line 24, in t
    return translate(key, locale=locale, **kwargs)
  File "/opt/collectd-reporting/lib/python3.7/site-packages/i18n/translator.py", line 44, in translate
    return TranslationFormatter(translation).format(**kwargs)
  File "/opt/collectd-reporting/lib/python3.7/site-packages/i18n/translator.py", line 16, in format
    return self.substitute(**kwargs)
  File "/usr/lib/python3.7/string.py", line 132, in substitute
    return self.pattern.sub(convert, self.template)
  File "/usr/lib/python3.7/string.py", line 129, in convert
    self._invalid(mo)
  File "/usr/lib/python3.7/string.py", line 105, in _invalid
    (lineno, colno))
ValueError: Invalid placeholder in string: line 1, col 13

First I looked at the units tests, but none where testing that config feature 😅

So I looked at the source code, and see that it uses the Template feature of the string module.
And the documentation state :

Note further that you cannot change the delimiter after class creation (i.e. a different delimiter must be set in the subclass’s class namespace).

Which is confirmed by the use of __init_subclass__(cls) in the Template source code

To my comprehension, because your module defines the class itself, despite it contains the following code, it will always get the value of the default delimiter as the moment of its definition (never get the chance to call i18n.set() before definition) :

class TranslationFormatter(Template):
    delimiter = config.get('placeholder_delimiter')

So I said, OK, I am just going to extend it with another class setting up my own delimiter (like this Template example), but it is impossible since you use that class hardcoded.

In @danhper code for i18n/translator.py:

def translate(key, **kwargs):
    locale = kwargs.pop('locale', config.get('locale'))
    translation = translations.get(key, locale=locale)
    if 'count' in kwargs:
        translation = pluralize(key, translation, kwargs['count'])
    return TranslationFormatter(translation).format(**kwargs)

In @ZRunner code for i18n/translator.py:

def translate(key: str, **kwargs) -> Union[List[Union[str, Any]], str]:
    """Actually translate something and apply plurals/kwargs if needed
    
    If the translation is a list of strings, each string will be formatted accordingly and the whole list will be returned"""
    locale = kwargs.pop('locale', config.get('locale'))
    translation = translations.get(key, locale=locale)
    if isinstance(translation, list):
        # if we can apply plural/formats to the items, let's try
        if all(isinstance(data, (str, dict)) for data in translation):
            # if needed, we should format every item in the list
            if 'count' in kwargs:
                translation = [pluralize(key, data, kwargs['count']) for data in translation]
            # items may be non-plural dictionnaries, which we cannot format
            return [TranslationFormatter(data).format(**kwargs) if isinstance(data, str) else data for data in translation]
        return translation
    if 'count' in kwargs:
        translation = pluralize(key, translation, kwargs['count'])
    return TranslationFormatter(translation).format(**kwargs)

So I recommend, as a solution, to allow in the config to specify a class, to be used by the translate() function.

In i18n/config.py

settings = {
  ...
  'class': TranslationFormatter,
  ...

In @danhper code for i18n/translator.py:

def translate(key, **kwargs):
    locale = kwargs.pop('locale', config.get('locale'))
    translation = translations.get(key, locale=locale)
    class_ref = config.get('class')
    if 'count' in kwargs:
        translation = pluralize(key, translation, kwargs['count'])
    return class_ref(translation).format(**kwargs)

And in In @ZRunner code for i18n/translator.py:

def translate(key: str, **kwargs) -> Union[List[Union[str, Any]], str]:
    """Actually translate something and apply plurals/kwargs if needed
    
    If the translation is a list of strings, each string will be formatted accordingly and the whole list will be returned"""
    locale = kwargs.pop('locale', config.get('locale'))
    translation = translations.get(key, locale=locale)
    class_ref = config.get('class')
    if isinstance(translation, list):
        # if we can apply plural/formats to the items, let's try
        if all(isinstance(data, (str, dict)) for data in translation):
            # if needed, we should format every item in the list
            if 'count' in kwargs:
                translation = [pluralize(key, data, kwargs['count']) for data in translation]
            # items may be non-plural dictionnaries, which we cannot format
            return [class_ref(data).format(**kwargs) if isinstance(data, str) else data for data in translation]
        return translation
    if 'count' in kwargs:
        translation = pluralize(key, translation, kwargs['count'])
    return class_ref(translation).format(**kwargs)

this way I would be able to use it like this :

>>> import i18n
>>> class MyDelimiter(i18n.translator.TranslationFormatter):
>>>     delimiter = '#'
>>> i18n.set('error_on_missing_placeholder', True)
>>> i18n.set('class', MyDelimiter)
>>> i18n.add_translation('hash', 'Hash: 10 #{value} !')
>>> i18n.t('hash', value='€')
'Hash: 10 € !'

Maybe that would be a little less efficient in terms of perfomance, but that would be way more flexible.

What do you think ?

Best regards.

[ZRunner]

Hi @mbideau and thanks for the feedback
I created a new branch with a fix for your issue right there: https://github.com/ZRunner/python-i18n/tree/custom-delimiter-fix
If you can try it and check that it works, I could merge it into my stable version

I also updated the readme (of the same branch) to reflect how you should edit the config, as I did it a bit differently than what you suggested

See you!

[mbideau]

Hi @ZRunner, thank you for taking the time to read and find a solution 😉

Sadly I couldn't make it work ...

❯ bin/pip install git+https://github.com/ZRunner/python-i18n.git@custom-delimiter-fix
Collecting git+https://github.com/ZRunner/python-i18n.git@custom-delimiter-fix
  Cloning https://github.com/ZRunner/python-i18n.git (to revision custom-delimiter-fix) to /tmp/pip-req-build-3nkcq3ys
  Running command git clone -q https://github.com/ZRunner/python-i18n.git /tmp/pip-req-build-3nkcq3ys
  Running command git checkout -b custom-delimiter-fix --track origin/custom-delimiter-fix
  Basculement sur la nouvelle branche 'custom-delimiter-fix'
  La branche 'custom-delimiter-fix' est paramétrée pour suivre la branche distante 'custom-delimiter-fix' depuis 'origin'.
Using legacy 'setup.py install' for python-i18n, since package 'wheel' is not installed.
Installing collected packages: python-i18n
    Running setup.py install for python-i18n ... done
Successfully installed python-i18n-0.3.12

❯ bin/python
Python 3.9.2 (default, Feb 28 2021, 17:03:44) 
[GCC 10.2.1 20210110] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import i18n
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/opt/testvenv/lib/python3.9/site-packages/i18n/__init__.py", line 3, in <module>
    from .translator import t
  File "/opt/testvenv/lib/python3.9/site-packages/i18n/translator.py", line 1, in <module>
    from string import Template, _TemplateMetaclass
ImportError: cannot import name '_TemplateMetaclass' from 'string' (/usr/lib/python3.9/string.py)

I search for the class in the Python source code, but couldn't find it either in version 3.9 nor 3.10, and the only commits mentioning it are from 2007 😅

Do you have an idea on how I can make it work ?

Thank you again.

[ZRunner]

Ok I think I found the issue and fixed it. For some reasons my computer used Python 3.8 during the tests so this specific import wasn't compatible with newer Python versions.

If I'm right the issue is now fixed (in a new commit on the custom-delimiter-fix branch) and should work on both 3.8 and newer versions (tested with 3.8.10 and 3.9.9)

[mbideau]

Hi @ZRunner,
Thank for the fix.
According to my tests, only the 'placeholder_delimiter' works, not the 'translation_formatter'. See the following:

❯ bin/pip install git+https://github.com/ZRunner/python-i18n.git@custom-delimiter-fix
Collecting git+https://github.com/ZRunner/python-i18n.git@custom-delimiter-fix
  Cloning https://github.com/ZRunner/python-i18n.git (to revision custom-delimiter-fix) to /tmp/pip-req-build-07oht2q7
  Running command git clone -q https://github.com/ZRunner/python-i18n.git /tmp/pip-req-build-07oht2q7
  Running command git checkout -b custom-delimiter-fix --track origin/custom-delimiter-fix
  Basculement sur la nouvelle branche 'custom-delimiter-fix'
  La branche 'custom-delimiter-fix' est paramétrée pour suivre la branche distante 'custom-delimiter-fix' depuis 'origin'.
Using legacy 'setup.py install' for python-i18n, since package 'wheel' is not installed.
Installing collected packages: python-i18n
    Running setup.py install for python-i18n ... done
Successfully installed python-i18n-0.3.12

❯ bin/python3
Python 3.9.2 (default, Feb 28 2021, 17:03:44) 
[GCC 10.2.1 20210110] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import i18n
>>> i18n.set('error_on_missing_placeholder', True)
>>> import string
>>> class MyTranslationFormatter(string.Template):
...     delimiter = '!'
... 
>>> i18n.set('translation_formatter', MyTranslationFormatter)
>>> i18n.add_translation('hash', 'Hash: 11 !{toto}')
>>> i18n.t('hash', toto='tata')
'Hash: 11 !{toto}'
>>> i18n.set('placeholder_delimiter', '!')
>>> i18n.t('hash', toto='tata')
'Hash: 11 tata'
>>> 

May be I am using it wrong ?

[ZRunner]

Woah thanks a lot, today I discovered a thing.
I forgot that isinstance could only check... well, instances of class, not class inheritance themselves. Apparently you can do that with issubclass, I never heard of it before.

Well anyway I tested it and my fix seems to work, at least for your above code. Don't hesitate to tell me if I forgot something, and thanks for your support!

[mbideau]

Hey @ZRunner now it is working.
But be careful of the lack of testing: now the 'translation_formatter' became required. See the following bug:

❯ bin/python3
Python 3.9.2 (default, Feb 28 2021, 17:03:44) 
[GCC 10.2.1 20210110] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import i18n
>>> i18n.set('error_on_missing_placeholder', True)
>>> i18n.add_translation('hash', 'Hash: 11 !{toto}')
>>> i18n.t('hash', toto='tata')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/opt/testvenv/lib/python3.9/site-packages/i18n/translator.py", line 34, in t
    return translate(key, locale=locale, **kwargs)
  File "/opt/testvenv/lib/python3.9/site-packages/i18n/translator.py", line 55, in translate
    TranslationFormatter: ModelTranslationFormatter = get_translation_formatter()
  File "/opt/testvenv/lib/python3.9/site-packages/i18n/translator.py", line 73, in get_translation_formatter
    if issubclass(custom_class, (Template, ModelTranslationFormatter)):
TypeError: issubclass() arg 1 must be a class
>>> import string
>>> class MyTranslationFormatter(string.Template):
...     delimiter = '!'
... 
>>> i18n.set('translation_formatter', MyTranslationFormatter)
>>> i18n.t('hash', toto='tata')
'Hash: 11 tata'
>>> 

Anyway, thank you very much for the fix 😉

[ZRunner]

Yup, I found the issue again
I just pushed the fix and added some tests for the new cases, everything seems to work (84 tests passing). Thanks for the reminder

I also took some time to fix the coverage verification CI, might make sure to add some other tests later (currently they "only" cover 94% of the code)

EDIT: branch merged on master

[mbideau]

Hey @ZRunner,
Thank you for fixing again 😉
Perfect 👌
94% is a very good coverage (weither it is statements, or LOC) 👍