Changeset View
Standalone View
pillar/cli/translations.py
- This file was added.
| import logging | |||||
| import os | |||||
sybren: Logging isn't used, so better to remove the import. | |||||
| import pathlib | |||||
| import subprocess | |||||
Not Done Inline ActionsKeep your imports of Python's stdlib together, without newlines, ordered alphabetically. sybren: Keep your imports of Python's stdlib together, without newlines, ordered alphabetically. | |||||
| from flask_script import Manager | |||||
| from pillar import current_app | |||||
| log = logging.getLogger(__name__) | |||||
| manager_translations = Manager( | |||||
| current_app, usage="Translation scripts, to create, update and compile localization") | |||||
| def get_application_folder() -> pathlib.Path: | |||||
| """ | |||||
| Folder to look for strings to translate | |||||
| """ | |||||
| return pathlib.Path(".") | |||||
sybrenUnsubmitted Not Done Inline ActionsWhy does this have to be a function? And why can it be relative? What is it relative to? Does it still work when the translation commands are *not* given from the top level directory of the project? sybren: Why does this have to be a function? And why can it be relative? What is it relative to? Does… | |||||
sybrenUnsubmitted Not Done Inline ActionsUse single quotes when possible. sybren: Use single quotes when possible. | |||||
Not Done Inline ActionsDoes this really return the pathlib module? sybren: Does this really return the pathlib module? | |||||
| def get_translations_folder() -> pathlib.Path: | |||||
| """ | |||||
| Returns folder where we store the translations | |||||
| """ | |||||
| return get_application_folder().joinpath('translations') | |||||
| def get_babel_settings() -> dict: | |||||
| """ | |||||
| Return a dict with all the required settings | |||||
Not Done Inline ActionsWhat should happen when an exception is raised? Shouldn't that also clean up messages.pot? According to the docstring it is, but the code doesn't do this. sybren: What should happen when an exception is raised? Shouldn't that also clean up messages.pot? | |||||
| for the translations commands | |||||
| """ | |||||
| ret_dict = { | |||||
sybrenUnsubmitted Done Inline ActionsI'm not too keen on using a dict for this. It makes it hard to figure out which keys there are, and half of the dict contents are hardcoded anyway, so could be module-level constants. The only two non-hardcoded values will in the end be turned into strings '.' and './translations'. It seems unclear to me why a new dict has to be constructed and three functions called, just to get these hardcoded settings that won't change between invocations. sybren: I'm not too keen on using a dict for this. It makes it hard to figure out which keys there are… | |||||
| 'babel_config': 'translations.cfg', | |||||
| 'messages_pot': 'messages.pot', | |||||
| 'application_folder': get_application_folder(), | |||||
| 'translations_folder': get_translations_folder(), | |||||
| } | |||||
| return ret_dict | |||||
| @manager_translations.command | |||||
Not Done Inline ActionsThe use of create_messages_pot() seems to consistently be create; run other command; cleanup. If the cleanup should always run, even when the "other command" fails, it would be better to turn create_messages_pot() into a context manager using @contextlib.contextmanager. sybren: The use of `create_messages_pot()` seems to consistently be create; run other command; cleanup. | |||||
| def init(locale): | |||||
| """ | |||||
| Initialize the translations for a new language. | |||||
| """ | |||||
| babel_settings = get_babel_settings() | |||||
| subprocess.call('pybabel extract -F {babel_config} -k lazy_gettext -o {messages_pot} {application_folder}'.format( | |||||
sybrenUnsubmitted Done Inline ActionsDon't pass the entire command as a string, this is very fragile and a potential security flaw. Use a list of individual arguments instead, and don't rely on str.split() to do this for you. For example, a slight change in get_application_folder() could result in an absolute path being used, which could contain spaces, breaking this command. sybren: Don't pass the entire command as a string, this is very fragile and a potential security flaw. | |||||
sybrenUnsubmitted Not Done Inline ActionsDon't use subprocess.call(), as it doesn't check the subprocess' return value. Use subprocess.run() instead, as this is [the recommended way to use the subprocess module](https://docs.python.org/3/library/subprocess.html#using-the-subprocess-module). sybren: Don't use `subprocess.call()`, as it doesn't check the subprocess' return value. Use… | |||||
| **babel_settings).split(' ')) | |||||
| subprocess.call('pybabel init -i {messages_pot} -d {translations_folder} -l {locale}'.format( | |||||
| **babel_settings, locale=locale).split(' ')) | |||||
| pathlib.Path('{messages_pot}'.format( | |||||
sybrenUnsubmitted Done Inline ActionsInstead of '{messages_pot}'.format(**babel_settings), just use babel_settings['messages_pot']. It's more readable, more explicit, and faster to execute. sybren: Instead of `'{messages_pot}'.format(**babel_settings)`, just use `babel_settings… | |||||
| **babel_settings)).unlink() | |||||
| @manager_translations.command | |||||
| def update(): | |||||
| """ | |||||
| Update the strings to be translated. | |||||
| """ | |||||
| babel_settings = get_babel_settings() | |||||
| subprocess.call('pybabel extract -F {babel_config} -k lazy_gettext -o {messages_pot} {application_folder}'.format( | |||||
sybrenUnsubmitted Done Inline ActionsThis is the exact same invocation as from the function above, so put it into its own Python function. sybren: This is the exact same invocation as from the function above, so put it into its own Python… | |||||
| **babel_settings).split(' ')) | |||||
| subprocess.call('pybabel update -i {messages_pot} -d {translations_folder}'.format( | |||||
| **babel_settings).split(' ')) | |||||
| pathlib.Path('{messages_pot}'.format( | |||||
| **babel_settings)).unlink() | |||||
Not Done Inline ActionsIt's clear that this is a list; available_modes would be a better name. Or even easier, just use the list literal directly at the choices=... argument, since it's used only once anyway. sybren: It's clear that this is a list; `available_modes` would be a better name. Or even easier, just… | |||||
| @manager_translations.command | |||||
| def compile(): | |||||
| """ | |||||
| Compile the translation to be used. | |||||
| """ | |||||
| babel_settings = get_babel_settings() | |||||
| subprocess.call('pybabel compile -d {translations_folder}'.format( | |||||
| **babel_settings).split(' ')) | |||||
| def parse_arguments(): | |||||
sybrenUnsubmitted Not Done Inline ActionsDeclare the return type sybren: Declare the return type | |||||
| """ | |||||
| When calling this script directly we need to handle the arguments | |||||
| as if we were a Pillar extension running via Flask cli | |||||
| """ | |||||
| import argparse | |||||
| list_of_modes = ['init', 'update', 'compile'] | |||||
| parser = argparse.ArgumentParser(description='Translate Pillar') | |||||
| parser.add_argument( | |||||
| 'mode', | |||||
| type=str, | |||||
| help='Init once, update often, compile before deploying.', | |||||
| choices=list_of_modes) | |||||
| parser.add_argument( | |||||
| 'languages', | |||||
| nargs='*', | |||||
| type=str, | |||||
| help='Languages to initialize: pt it es ...') | |||||
| args = parser.parse_args() | |||||
| if args.mode == 'init' and not args.languages: | |||||
| parser.error("init requires languages") | |||||
| return args | |||||
| def main(): | |||||
| """ | |||||
| For Pillar we need to run this via command-line, without relying on Flask-cli | |||||
sybrenUnsubmitted Not Done Inline ActionsWhat does "For Pillar" mean? sybren: What does "For Pillar" mean? | |||||
| So we parse the arguments, and init/update/compile the translations strings | |||||
| """ | |||||
| args = vars(parse_arguments()) | |||||
sybrenUnsubmitted Done Inline ActionsWhy do you need a vars() call here? Just use args.mode and args.languages directly. sybren: Why do you need a `vars()` call here? Just use `args.mode` and `args.languages` directly.
| |||||
| mode = args['mode'] | |||||
| if mode == 'init': | |||||
| for language in args['languages']: | |||||
| init(language) | |||||
| elif mode == 'update': | |||||
| update() | |||||
| else: # mode == 'compile' | |||||
| compile() | |||||
| if __name__ == '__main__': | |||||
| main() | |||||
Logging isn't used, so better to remove the import.