Differential D2826 Diff 9232 pillar/cli/translations.py

Changeset View

Standalone View

pillar/cli/translations.py

This file was added.

				import logging

				import argparse
				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 pathlib
				import subprocess

				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

				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
				sybrenUnsubmitted Not Done Inline Actions Does this really return the pathlib module? sybren: Does this really return the pathlib module?

				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.
				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):
				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?
				"""Delete the temporarily created messages.pot file
				"""
				messages_pot.unlink()

				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…

				@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))
				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.
				clean_messages_pot(messages_pot)


				@manager_translations.command
				def update():
				"""
				Update the strings to be translated.
				"""
				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…
				messages_pot = create_messages_pot()
				subprocess.run(('pybabel', 'update', '-i', messages_pot, '-d', 'translations'))
				clean_messages_pot(messages_pot)


				@manager_translations.command
				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…
				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:
				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…
				"""
				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']

				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 = 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()
				sybrenUnsubmitted Not Done Inline Actions Declare 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()

				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.