Differential D2826 Diff 9238 pillar/cli/translations.py

Changeset View

Standalone View

pillar/cli/translations.py

This file was added.

				import argparse
				import pathlib
				import subprocess
				sybrenUnsubmitted Not Done Inline Actions Logging isn't used, so better to remove the import. sybren: Logging isn't used, so better to remove the import.

				import contextlib
				sybrenUnsubmitted Not Done Inline Actions Keep 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

				manager_translations = Manager(
				current_app, usage="Translation scripts, to create, update and compile localization")


				BABEL_CONFIG = 'translations.cfg'


				@contextlib.contextmanager
				def create_messages_pot() -> pathlib.Path:
				"""Extract the translatable strings from the source code

				sybrenUnsubmitted Not Done Inline Actions Does this really return the pathlib module? sybren: Does this really return the pathlib module?
				This creates a temporary messages.pot file, to be used to init or
				sybrenUnsubmitted Not Done Inline Actions Why 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 Actions Use single quotes when possible. sybren: Use single quotes when possible.
				update the translation .mo files.

				It works as a generator, yielding the temporarily created pot file.
				The messages.pot file is deleted at the end of it.

				: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, '.'))
				yield messages_pot
				sybrenUnsubmitted Not Done Inline Actions What 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?
				messages_pot.unlink()


				@manager_translations.command
				def init(locale):
				sybrenUnsubmitted Done Inline Actions I'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…
				"""
				Initialize the translations for a new language.
				"""
				with create_messages_pot() as messages_pot:
				subprocess.run(('pybabel', 'init', '-i', messages_pot, '-d', 'translations', '-l', locale))


				@manager_translations.command
				sybrenUnsubmitted Not Done Inline Actions The 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 update():
				"""
				Update the strings to be translated.
				"""
				with create_messages_pot() as messages_pot:
				subprocess.run(('pybabel', 'update', '-i', messages_pot, '-d', 'translations'))


				sybrenUnsubmitted Done Inline Actions Don'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 Actions Don'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…
				@manager_translations.command
				def compile():
				"""
				Compile the translation to be used.
				"""
				if pathlib.Path('translations').is_dir():
				sybrenUnsubmitted Done Inline Actions Instead 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…
				subprocess.run(('pybabel', 'compile','-d', 'translations'))
				else:
				print("No translations folder available")


				def parse_arguments() -> argparse.Namespace:
				"""
				When calling this script directly we need to handle the arguments
				as if we were a Pillar extension running via Flask cli
				"""
				parser = argparse.ArgumentParser(description='Translate Pillar')
				sybrenUnsubmitted Done Inline Actions This 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…

				parser.add_argument(
				'mode',
				type=str,
				help='Init once, update often, compile before deploying.',
				choices=['init', 'update', 'compile'])
				sybrenUnsubmitted Not Done Inline Actions It'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.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():
				sybrenUnsubmitted Not Done Inline Actions Declare the return type sybren: Declare the return type
				"""
				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()

				sybrenUnsubmitted Not Done Inline Actions What does "For Pillar" mean? sybren: What does "For Pillar" mean?
				sybrenUnsubmitted Done Inline Actions Why 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.