Page MenuHome

Support localization of strings based on user browser
ClosedPublic

Authored by Dalai Felinto (dfelinto) on Sep 5 2017, 9:11 PM.

Diff Detail

Repository
rPS Pillar

Event Timeline

.gitignore
25

Does this intentionally not start with a slash? Will translations be in any other directory than the top-level translations dir?

(and yes I know this also applies to the lines below, but those are there already ;-) )

README.md
37

Running code from inside the source tree like this isn't that nice. Can't it be an entry point defined in setup.py?

79

Can you make those "look at" targets links to (a webpage showing) their actual sources?

pillar/__init__.py
262

You can just use pathlib.Path(self.app_root) / 'translations' instead of calling joinpath()

263

No need to construct a list to pass it to languages.extend(), use a generator expression instead.

289

This inner function isn't called here, and it looks like it will just immediately be garbage collected. My guess is that somehow this is prevented by the @babel.localeselector annotation, but this should be made explicit.

290

Annotate the return type.

294

Why is it necessary to both return the locale *and* set it on g.locale? This should be included in the docstring, as it hints as how the entire translation system integrates with the rest of Pillar.

369

I18 is an incomplete name, it should be I18N, which is short for "internationalization".
Also flip the condition and return early, so

if not self.config['USE_I18N'}:
    return

That'll allow you to un-indent the remainder of this function.

371

Same as above, can be flipped to if not trpath: return, un-indenting the remainder of the function by another level.

375

The message isn't reflecting the code. trpath can exist as a file, symlink, or socket, and the message will tell you it doesn't exist at all.

375

Use f-strings for formatting, so f'Translation path {trpath} for extension {pillar_extension.name} does not exist'
Don't end exception messages in a period.

pillar/cli/translations.py
22

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?

22

Use single quotes when possible.

37

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.

53

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.

53

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.

59

Instead of '{messages_pot}'.format(**babel_settings), just use babel_settings['messages_pot']. It's more readable, more explicit, and faster to execute.

70

This is the exact same invocation as from the function above, so put it into its own Python function.

91

Declare the return type

122

What does "For Pillar" mean?

125

Why do you need a vars() call here? Just use args.mode and args.languages directly.

pillar/config.py
194

See earlier comment, should be USE_I18N. If you really want to be a special snowflake, at least use USE_I19 ;-)
FYI, "i18n" just means "an 'i' and an 'n' with 18 letters in between".

195

This needs documentation about allowed values. For example, a locale can also include a territory, a codeset, and a modifier. Especially territories are important, because nl_NL is not the same language as nl_BE.

pillar/extension.py
123

Returning "" is in violation of the declared return type.

126

Why use parents[1]? Does this always exist?

128

Returning None is in violation of the declared return type and the docstring.

setup.py
12

Use super().run() instead.

translations.cfg
2

Does this file format support comments? If so, add a description of what the file is for, and where the syntax of this file can be found.

Sybren A. Stüvel (sybren) requested changes to this revision.Sep 6 2017, 10:36 AM
This revision now requires changes to proceed.Sep 6 2017, 10:36 AM
Dalai Felinto (dfelinto) edited edge metadata.
Dalai Felinto (dfelinto) marked 23 inline comments as done.

All the changes suggested by review.

  • Touch ups on documentation and README.md
  • We can extract the strings directly from the pug files

I'm thinking here, maybe USE_I18N shouldn't exist at all. Given that if you don't have any translations folder, PillarServer.languages will be {'en_US'}. We still need babel running and everything even in this case, otherwise the strings markup "gettext( )", "_( )" will throw errors.

README.md
79

You mean:
(1) https://docs.python.org/3/library/gettext.html
(2) http://babel.pocoo.org/en/latest/messages.html
or
(3) The internal file: pillar/__init__.py -> return _('The submitted data could not be validated.'), 422?

pillar/config.py
194

L6G S7G N1W E3Y D1Y T3K Y1U!

README.md
79

I mean whatever you mean with "look at the Python code". If you tell people to look at something, it's nice to let them know where they can find it.

pillar/__init__.py
263

You can drop the superfluous parentheses.

300

An if not with an else clause is hard to read. This would be simpler to parse mentally:

if self.config['USE_I18N']:
    g.locale = request.accept_languages.best_match(self.languages, self.default_locale)
else:
    g.locale = 'en_US'
return g.locale

Every access to g requires going through a LocalProxy object. It'll be faster to determine the locale and store it in a local variable, then assign that to g.locale and return the local variable:

if self.config['USE_I18N']:
    locale = request.accept_languages.best_match(self.languages, self.default_locale)
else:
    locale = 'en_US'
g.locale = locale
return locale

Since this happens on every request, performance is important.

370

Add a debug log entry that explains it's going to skip I18N stuff.

374

Add a debug log entry that explains this extension doesn't have a translations path

378

This can never happen, given how the translations_path property is written.

pillar/cli/translations.py
3

Logging isn't used, so better to remove the import.

21

Does this really return the pathlib module?

45

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.

76

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.

pillar/config.py
196

Are different codesets supported by Pillar then? Don't we always assume UTF-8? And what do those modifiers do? If you include this text, it's important to know what will work and what won't.

From review:

  • Sorted changes
  • Better DEFAULT_LOCALE documentation
  • Changes on README.md

Also:

  • Remove USE_I18N. If an extension doesn't want internationalization, just don't add any translations folder.
pillar/__init__.py
371

Don't use f-strings in logging. Now the string will be formatted even before doing the self.log.debug() call, which might drop the formatted string whenever debug-level logging is disabled. Use percent formatting and pass format arguments as extra args to self.log.debug().

pillar/cli/translations.py
6

Keep your imports of Python's stdlib together, without newlines, ordered alphabetically.

33

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.

pillar/config.py
196

Maybe mention that all translations should be in UTF-8?

  • From review: all suggested fixes
  • Fix for errors when main project has no translations folder
  • Fix missing error when no translations.cfg was found
This revision is now accepted and ready to land.Sep 8 2017, 6:49 PM
This revision was automatically updated to reflect the committed changes.