Issue 345741002: trytond: Improve translation handling for plain text templates
DescriptionPatch Set 1 #
Total comments: 9
Patch Set 2 : Fix comments #
Total comments: 4
Patch Set 3 : Refactor based on the comments #
Total comments: 2
Patch Set 4 : Split callback function, remove dict as default #
Total comments: 16
Patch Set 5 : Update to tip, fix some remarks #
Total comments: 2
Patch Set 6 : Fix remarks, add translator to report context #Patch Set 7 : Fix isort error #
Total comments: 6
Patch Set 8 : Fix comments, add changelog #Patch Set 9 : Remove double space #
MessagesTotal messages: 32
flake8 OK URL: https://codereview.tryton.org/345741002
Sign in to reply to this message.
https://codereview.tryton.org/345741002/diff/345751002/CHANGELOG File CHANGELOG (right): https://codereview.tryton.org/345741002/diff/345751002/CHANGELOG#newcode1 CHANGELOG:1: * Improve translation handling for plain text templates No need for changelog entry when improving the existing. https://codereview.tryton.org/345741002/diff/345751002/trytond/ir/translation.py File trytond/ir/translation.py (right): https://codereview.tryton.org/345741002/diff/345751002/trytond/ir/translation... trytond/ir/translation.py:16: from genshi.filters.i18n import extract as genshi_extract, GETTEXT_FUNCTIONS GETTEXT_FUNCTIONS is not a public variable, we should not rely on it. https://codereview.tryton.org/345741002/diff/345751002/trytond/ir/translation... trytond/ir/translation.py:949: options['extract_text'] = False What about non text format, they must be extracted. https://codereview.tryton.org/345741002/diff/345751002/trytond/report/report.py File trytond/report/report.py (right): https://codereview.tryton.org/345741002/diff/345751002/trytond/report/report.... trytond/report/report.py:292: return None No need to return None https://codereview.tryton.org/345741002/diff/345751002/trytond/report/report.... trytond/report/report.py:305: report_context.setdefault('_', translate) I think it is better to not define it if translate is None. It will give a more explicit error if it is used in the template.
Sign in to reply to this message.
Fixed the comments. Because we use only the '_' for translation, I decided to only search for that function instead of all the other GETTEXT_FUNCTIONS https://codereview.tryton.org/345741002/diff/345751002/trytond/ir/translation.py File trytond/ir/translation.py (right): https://codereview.tryton.org/345741002/diff/345751002/trytond/ir/translation... trytond/ir/translation.py:16: from genshi.filters.i18n import extract as genshi_extract, GETTEXT_FUNCTIONS On 2021/04/05 14:39:57, ced wrote: > GETTEXT_FUNCTIONS is not a public variable, we should not rely on it. Done. https://codereview.tryton.org/345741002/diff/345751002/trytond/ir/translation... trytond/ir/translation.py:949: options['extract_text'] = False On 2021/04/05 14:39:58, ced wrote: > What about non text format, they must be extracted. Done. https://codereview.tryton.org/345741002/diff/345751002/trytond/report/report.py File trytond/report/report.py (right): https://codereview.tryton.org/345741002/diff/345751002/trytond/report/report.... trytond/report/report.py:292: return None On 2021/04/05 14:39:58, ced wrote: > No need to return None Done. https://codereview.tryton.org/345741002/diff/345751002/trytond/report/report.... trytond/report/report.py:305: report_context.setdefault('_', translate) On 2021/04/05 14:39:58, ced wrote: > I think it is better to not define it if translate is None. It will give a more > explicit error if it is used in the template. Done.
Sign in to reply to this message.
https://codereview.tryton.org/345741002/diff/361521004/trytond/ir/translation... trytond/ir/translation.py:47: expected 2 blank lines, found 1 URL: https://codereview.tryton.org/345741002
Sign in to reply to this message.
https://codereview.tryton.org/345741002/diff/361521004/trytond/report/report.py File trytond/report/report.py (right): https://codereview.tryton.org/345741002/diff/361521004/trytond/report/report.... trytond/report/report.py:305: report_context.setdefault('_', translate) Should we use GETTEXT FUNCTIONS from translate?
Sign in to reply to this message.
https://codereview.tryton.org/345741002/diff/361521004/trytond/report/report.py File trytond/report/report.py (right): https://codereview.tryton.org/345741002/diff/361521004/trytond/report/report.... trytond/report/report.py:305: report_context.setdefault('_', translate) On 2021/04/06 12:04:01, pokoli wrote: > Should we use GETTEXT FUNCTIONS from translate? Hmm, not a bad idea, this can then be converted into a for loop to set the defaults.
Sign in to reply to this message.
https://codereview.tryton.org/345741002/diff/361521004/trytond/ir/translation.py File trytond/ir/translation.py (right): https://codereview.tryton.org/345741002/diff/361521004/trytond/ir/translation... trytond/ir/translation.py:45: GETTEXT_FUNCTIONS = ('_') I think it makes more sense to define it on report.py https://codereview.tryton.org/345741002/diff/361521004/trytond/ir/translation... trytond/ir/translation.py:946: def extract_report_genshi(template_class, extract_text=True): For me it will be cleaner to pass the options and keywords as arguments.
Sign in to reply to this message.
https://codereview.tryton.org/345741002/diff/317051002/trytond/report/report.py File trytond/report/report.py (left): https://codereview.tryton.org/345741002/diff/317051002/trytond/report/report.... trytond/report/report.py:291: Maybe it's better to split the _callback_loader? For the plain text template only the TranslateFactory is needed.
Sign in to reply to this message.
flake8 OK URL: https://codereview.tryton.org/345741002
Sign in to reply to this message.
https://codereview.tryton.org/345741002/diff/317051002/trytond/ir/translation.py File trytond/ir/translation.py (right): https://codereview.tryton.org/345741002/diff/317051002/trytond/ir/translation... trytond/ir/translation.py:946: def extract_report_genshi(template_class, keywords=None, options={}): You need to be careful using a dictionary as a default value: https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments
Sign in to reply to this message.
flake8 OK URL: https://codereview.tryton.org/345741002
Sign in to reply to this message.
Missing a changelog entry. https://codereview.tryton.org/345741002/diff/357691002/trytond/ir/translation.py File trytond/ir/translation.py (right): https://codereview.tryton.org/345741002/diff/357691002/trytond/ir/translation... trytond/ir/translation.py:950: keywords=keywords, comment_tags=None, options=options): Why not keep options as keyword argument? https://codereview.tryton.org/345741002/diff/357691002/trytond/ir/translation... trytond/ir/translation.py:967: factories.get('text'), Why using .get? https://codereview.tryton.org/345741002/diff/357691002/trytond/ir/translation... trytond/ir/translation.py:970: ) This is a strange style. https://codereview.tryton.org/345741002/diff/357691002/trytond/ir/translation... trytond/ir/translation.py:976: factories.get('markup', factories.get('xml'))) I think we could also add GETTEXT_FUNCTIONS as keyword for markup. https://codereview.tryton.org/345741002/diff/357691002/trytond/report/report.py File trytond/report/report.py (right): https://codereview.tryton.org/345741002/diff/357691002/trytond/report/report.... trytond/report/report.py:91: GETTEXT_FUNCTIONS = ('_', 'gettext') For me it makes more sense to use a list here because it is not a kind of struct. https://codereview.tryton.org/345741002/diff/357691002/trytond/report/report.... trytond/report/report.py:282: def _callback_translator(cls): I do not understand why it is named callback? It is just the constructor of translate. https://codereview.tryton.org/345741002/diff/357691002/trytond/report/report.... trytond/report/report.py:292: translator = Translator(lambda text: translate(text)) 'extract_text' should not be set for txt template. https://codereview.tryton.org/345741002/diff/357691002/trytond/report/report.... trytond/report/report.py:313: report_context.setdefault(t, translate) I think the gettext function should be set in get_context for any format of template. It may be useful for example in XML to call gettext inside some expression.
Sign in to reply to this message.
https://codereview.tryton.org/345741002/diff/357691002/trytond/report/report.py File trytond/report/report.py (right): https://codereview.tryton.org/345741002/diff/357691002/trytond/report/report.... trytond/report/report.py:91: GETTEXT_FUNCTIONS = ('_', 'gettext') On 2021/05/27 21:41:51, ced wrote: > For me it makes more sense to use a list here because it is not a kind of > struct. Done. https://codereview.tryton.org/345741002/diff/357691002/trytond/report/report.... trytond/report/report.py:282: def _callback_translator(cls): On 2021/05/27 21:41:51, ced wrote: > I do not understand why it is named callback? > It is just the constructor of translate. I'm always struggling with naming. I have now taken the name of the class. https://codereview.tryton.org/345741002/diff/357691002/trytond/report/report.... trytond/report/report.py:292: translator = Translator(lambda text: translate(text)) On 2021/05/27 21:41:51, ced wrote: > 'extract_text' should not be set for txt template. I don't understand. https://codereview.tryton.org/345741002/diff/357691002/trytond/report/report.... trytond/report/report.py:313: report_context.setdefault(t, translate) On 2021/05/27 21:41:51, ced wrote: > I think the gettext function should be set in get_context for any format of > template. It may be useful for example in XML to call gettext inside some > expression. Those expressions are already taken by the translation system right? Does this not interfere with the translation system? Also shouldn't you use the i18n directive in that case?
Sign in to reply to this message.
https://codereview.tryton.org/345741002/diff/357691002/trytond/report/report.py File trytond/report/report.py (right): https://codereview.tryton.org/345741002/diff/357691002/trytond/report/report.... trytond/report/report.py:292: translator = Translator(lambda text: translate(text)) On 2022/10/03 15:52:29, EdbO wrote: > On 2021/05/27 21:41:51, ced wrote: > > 'extract_text' should not be set for txt template. > > I don't understand. extract_text is an option of Translator. So for me for txt template we should not try to translate plain text but only part that called by gettext. https://codereview.tryton.org/345741002/diff/357691002/trytond/report/report.... trytond/report/report.py:313: report_context.setdefault(t, translate) On 2022/10/03 15:52:29, EdbO wrote: > On 2021/05/27 21:41:51, ced wrote: > > I think the gettext function should be set in get_context for any format of > > template. It may be useful for example in XML to call gettext inside some > > expression. > > Those expressions are already taken by the translation system right? I do not understand > Does this > not interfere with the translation system? How? > Also shouldn't you use the i18n > directive in that case? i18n is to translate a part of XML, here I'm talking for example content of an attribute. https://codereview.tryton.org/345741002/diff/437441003/trytond/ir/translation.py File trytond/ir/translation.py (right): https://codereview.tryton.org/345741002/diff/437441003/trytond/ir/translation... trytond/ir/translation.py:984: def extract_report_genshi(template_class, keywords=None, options=None): why not use **options https://codereview.tryton.org/345741002/diff/437441003/trytond/ir/translation... trytond/ir/translation.py:986: keywords=keywords, comment_tags=None, **options): options will not still not be set to the one passed just above.
Sign in to reply to this message.
ERROR: /tmp/reviewbot-6a36p0mx/trytond/report/__init__.py Imports are incorrectly sorted and/or formatted. URL: https://codereview.tryton.org/345741002
Sign in to reply to this message.
https://codereview.tryton.org/345741002/diff/357691002/trytond/report/report.py File trytond/report/report.py (right): https://codereview.tryton.org/345741002/diff/357691002/trytond/report/report.... trytond/report/report.py:292: translator = Translator(lambda text: translate(text)) On 2022/10/03 16:04:31, ced wrote: > On 2022/10/03 15:52:29, EdbO wrote: > > On 2021/05/27 21:41:51, ced wrote: > > > 'extract_text' should not be set for txt template. > > > > I don't understand. > > extract_text is an option of Translator. > So for me for txt template we should not try to translate plain text but only > part that called by gettext. Done. https://codereview.tryton.org/345741002/diff/357691002/trytond/report/report.... trytond/report/report.py:313: report_context.setdefault(t, translate) On 2022/10/03 16:04:31, ced wrote: > On 2022/10/03 15:52:29, EdbO wrote: > > On 2021/05/27 21:41:51, ced wrote: > > > I think the gettext function should be set in get_context for any format of > > > template. It may be useful for example in XML to call gettext inside some > > > expression. > > > > Those expressions are already taken by the translation system right? > > I do not understand > > > Does this > > not interfere with the translation system? > > How? <span py:when="sale.state == 'quotation'" i18n:msg="quotation,number"> ${_('Quotation N°')}: ${sale.full_number} testtext </span> Tryton / Genshi will extract the complete sentence and removes the _(..) So as long as you don't use two together in one sentence, it will work. I also think this is an edge case which doesn't make sense. > > > Also shouldn't you use the i18n > > directive in that case? > > i18n is to translate a part of XML, here I'm talking for example content of an > attribute. Did some testing and it works the same as with the TEXT templates.
Sign in to reply to this message.
On 2021/05/27 21:41:52, ced wrote: > Missing a changelog entry. In the first patch set you said there was no need to add a changelog entry. So is a changelog entry needed? Also thinking about documentation. Because there are a few things needed to get translation working in the templates like the set_lang functions. With XML and HTML you also some extra attributes in the <html> tags. Maybe in another issue?
Sign in to reply to this message.
ERROR: /tmp/reviewbot-eonp9pbr/trytond/report/__init__.py Imports are incorrectly sorted and/or formatted. URL: https://codereview.tryton.org/345741002
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/345741002
Sign in to reply to this message.
https://codereview.tryton.org/345741002/diff/425811003/trytond/ir/translation.py File trytond/ir/translation.py (right): https://codereview.tryton.org/345741002/diff/425811003/trytond/ir/translation... trytond/ir/translation.py:986: keywords=keywords, comment_tags=None, options=options): You can not change the API of this method, it must stay the same ad the genshi_extract. https://codereview.tryton.org/345741002/diff/425811003/trytond/ir/translation... trytond/ir/translation.py:1005: options={'extract_text': False}) This can not work, options is not a keyword. https://codereview.tryton.org/345741002/diff/425811003/trytond/report/report.py File trytond/report/report.py (right): https://codereview.tryton.org/345741002/diff/425811003/trytond/report/report.... trytond/report/report.py:323: report_context[t] = translate maybe simpler: report_context.update(dict.fromkeys(GETTEXT_FUNCTIONS, cls._translate_factory())) https://codereview.tryton.org/345741002/diff/425811003/trytond/report/report.... trytond/report/report.py:328: def _translate_factory(cls): Not sure we need a function for that. We could just put the pool.get in the __init__ of TranslateFactory https://codereview.tryton.org/345741002/diff/425811003/trytond/report/report.... trytond/report/report.py:340: extract_text = False could be written: extract_text = report.template_extension != 'txt' https://codereview.tryton.org/345741002/diff/425811003/trytond/report/report.... trytond/report/report.py:341: translator = Translator(lambda text: translate(text), break line at opening (
Sign in to reply to this message.
On 2022/10/03 18:59:33, EdbO wrote: > On 2021/05/27 21:41:52, ced wrote: > > Missing a changelog entry. > > In the first patch set you said there was no need to add a changelog entry. > So is a changelog entry needed? Well now it adds new feature. > Also thinking about documentation. Because there are a few things needed to > get translation working in the templates like the set_lang functions. With > XML and HTML you also some extra attributes in the <html> tags. > > Maybe in another issue? Yes.
Sign in to reply to this message.
https://codereview.tryton.org/345741002/diff/435911045/trytond/report/report.... trytond/report/report.py:331: multiple spaces after operator URL: https://codereview.tryton.org/345741002
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/345741002
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
