Changeset View
Standalone View
pillar/cli/translations.py
- This file was added.
| import logging | |||||
| import argparse | |||||
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") | |||||
| BABEL_CONFIG = 'translations.cfg' | |||||
| def create_messages_pot() -> pathlib: | |||||
| """Extract the translatable strings from the source code | |||||
Not Done Inline ActionsDoes this really return the pathlib module? sybren: Does this really return the pathlib module? | |||||
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… | |||||
Not Done Inline ActionsUse single quotes when possible. sybren: Use single quotes when possible. | |||||
| This creates a temporary messages.pot file, to be used to init or | |||||
| update the translation .mo files. | |||||
| :return The path of the messages.pot file created. | |||||
| """ | |||||
| messages_pot = pathlib.Path('messages.pot') | |||||
| subprocess.run(('pybabel', 'extract', '-F', BABEL_CONFIG, '-k', 'lazy_gettext', '-o', messages_pot, '.')) | |||||
| return messages_pot | |||||
| def clean_messages_pot(messages_pot: pathlib.Path): | |||||
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? | |||||
| """Delete the temporarily created messages.pot file | |||||
| """ | |||||
| messages_pot.unlink() | |||||
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… | |||||
| @manager_translations.command | |||||
| def init(locale): | |||||
| """ | |||||
| Initialize the translations for a new language. | |||||
| """ | |||||
| messages_pot = create_messages_pot() | |||||
| subprocess.run(('pybabel', 'init', '-i', messages_pot, '-d', 'translations', '-l', locale)) | |||||
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. | |||||
| clean_messages_pot(messages_pot) | |||||
| @manager_translations.command | |||||
| def update(): | |||||
| """ | |||||
| Update the strings to be translated. | |||||
| """ | |||||
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. | |||||
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… | |||||
| messages_pot = create_messages_pot() | |||||
| subprocess.run(('pybabel', 'update', '-i', messages_pot, '-d', 'translations')) | |||||
| clean_messages_pot(messages_pot) | |||||
| @manager_translations.command | |||||
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… | |||||
| def compile(): | |||||
| """ | |||||
| Compile the translation to be used. | |||||
| """ | |||||
| if pathlib.Path('translations').is_dir(): | |||||
| subprocess.run(('pybabel', 'compile','-d', 'translations')) | |||||
| else: | |||||
| print("No translations folder available") | |||||
| def parse_arguments() -> argparse.Namespace: | |||||
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… | |||||
| """ | |||||
| When calling this script directly we need to handle the arguments | |||||
| as if we were a Pillar extension running via Flask cli | |||||
| """ | |||||
| list_of_modes = ['init', 'update', 'compile'] | |||||
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… | |||||
| 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() | |||||
Not Done Inline ActionsDeclare the return type sybren: Declare the return type | |||||
| if args.mode == 'init' and not args.languages: | |||||
| parser.error("init requires languages") | |||||
| return args | |||||
| def main(): | |||||
| """ | |||||
| When calling from the setup.py entry-point we need to parse the arguments | |||||
| and init/update/compile the translations strings | |||||
| """ | |||||
| args = parse_arguments() | |||||
| if args.mode == 'init': | |||||
| for language in args.languages: | |||||
| init(language) | |||||
| elif args.mode == 'update': | |||||
| update() | |||||
| else: # mode == 'compile' | |||||
| compile() | |||||
| if __name__ == '__main__': | |||||
| main() | |||||
Not Done Inline ActionsWhat does "For Pillar" mean? sybren: What does "For Pillar" mean? | |||||
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.
| |||||
Logging isn't used, so better to remove the import.