Opened 13 years ago

Closed 7 years ago

#17379 closed Cleanup/optimization (fixed)

Don't deactivate translations by default in management commands

Reported by: Chris Chambers Owned by: Ramiro Morales
Component: Internationalization Version: dev
Severity: Normal Keywords: management, shell, i18n, l10n, en-us, translation, documentation
Cc: lpiatek, neaf Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I see the rationale in django.management.core.base, but this is unexpected behaviour for ./manage.py shell, for example - particularly if you're running with a LANGUAGE_CODE like 'en-gb' as there's no visual feedback/documentation to illustrate that l10n/i18n are off. Granted, this won't make affect many people, but it can cause all kinds of debugging woes.

I'd like to suggest the simple fix of either:

  1. forcing translation.activate(settings.LANGUAGE_CODE) during the ./manage.py shell command.
  2. Adding some visual feedback/documentation if this turns out to be impractical for whatever reason.

Attachments (4)

17379-1.diff (1.8 KB ) - added by Claude Paroz 13 years ago.
Add switch to commands to keep active language
17379-2.diff (2.0 KB ) - added by lpiatek 13 years ago.
17379-3.diff (5.6 KB ) - added by lpiatek 13 years ago.
decouple can_import_settings from force_en_us_locale
17379-4.diff (7.0 KB ) - added by gabooo 13 years ago.
Fix i18n makemessages command

Download all attachments as: .zip

Change History (28)

comment:1 by Chris Chambers, 13 years ago

Type: UncategorizedBug

comment:2 by Chris Chambers, 13 years ago

See also: #11648, which might require the same fix (albeit for a different reason) if the underlying issue isn't/can't be corrected (#9340).

Last edited 9 years ago by Tim Graham (previous) (diff)

by Claude Paroz, 13 years ago

Attachment: 17379-1.diff added

Add switch to commands to keep active language

comment:3 by Claude Paroz, 13 years ago

Has patch: set
Needs documentation: set
Triage Stage: UnreviewedAccepted
Version: 1.3SVN

Attached patch with a possible approach to the problem.

by lpiatek, 13 years ago

Attachment: 17379-2.diff added

comment:4 by lpiatek, 13 years ago

Comment on https://code.djangoproject.com/attachment/ticket/17379/17379-1.diff patch:

  • keep_active_language is not required, can_import_settings is only used for what we want to turn off. So maybe we do not need new flag?
  • printing sholdn't be done on stderr, but on stdout?
  • we changed information to: "Active language set to 'en-us'.\n"

Comment on whole:

  • there should be in BaseCommand Class functions like: _pre_execute(self), _post_execute(self) when such things like translations would be run, but this kind of change requires 'Design Decision', so we ommit it for now

#djangosprint-krakow

comment:5 by Claude Paroz, 13 years ago

Patch needs improvement: set

Totally agree about changes related to stderr/stdout.

However I don't see how can_import_settings and keep_active_language are related. For me, those are two different unrelated flags, or am I missing something?

    ``can_import_settings``
        A boolean indicating whether the command needs to be able to
        import Django settings; if ``True``, ``execute()`` will verify
        that this is possible before proceeding. Default value is
        ``True``.

In some cases, we probably will have commands with can_import_settings = True, but where we don't want to switch to en-us.

by lpiatek, 13 years ago

Attachment: 17379-3.diff added

decouple can_import_settings from force_en_us_locale

by gabooo, 13 years ago

Attachment: 17379-4.diff added

Fix i18n makemessages command

comment:6 by Ramiro Morales, 13 years ago

I'd like to ask to the contributors working on this ticket what are the practical consequences (as in concrete use cases) for the 'shell'and 'makemessages' of being run always with the en-us locale.

It is not clear to me why both of them are particular among the rest of the commands and IMHO having that information would be helpful when it comes to solve/re-triage this ticket.

comment:7 by neaf, 13 years ago

The ticket requested shell to respect locale setting. Since it's an interface to your app there's no reason not to do that. You might want to see how your locale or other functionality that depends on i18n behaves.

can_import_settings was supposed to check if settings are available before execution of a command according to documentation (https://docs.djangoproject.com/en/dev/howto/custom-management-commands/#BaseCommand.can_import_settings). It was doing that only by coincidence… it was doing that by forcing en_us locale via translation module which relies on settings.

makemessages used that coincidental behavior to avoid forced locale and respect language set in your settings file.

The patch decouples can_import_settings from the locale forcing behavior to allow its proper usage as described in documentation.

makemessages implementation didn't change. The only difference is it now uses force_en_us_locale instead of can_import_settings to achieve the same thing.

comment:8 by lpiatek, 13 years ago

Patch needs improvement: unset

comment:9 by Ramiro Morales, 13 years ago

Patch needs improvement: set

Thanks for the explanation.

I'd like to:

  • Review if raising StandardError when force_en_us_locale = True and can_import_settings = False is the right thing to do
  • Decide if compilemessages (the only other one command that sets can_import_settings = False) does that because it needs to load settings or as a side effect of needind to activate locale, just as makemessages did
Version 0, edited 13 years ago by Ramiro Morales (next)

comment:10 by lpiatek, 13 years ago

Cc: lpiatek added

comment:11 by neaf, 13 years ago

Cc: neaf added

comment:12 by lpiatek, 13 years ago

Raising StandardError when force_en_us_locale = True and can_import_settings = False:

  • for me it's wrong cause force_en_us_locale according to patch is set to True by default, so any command which now has can_import_settings = False would throw mentioned Exception. We just can't assume we need

Possible options:

  • set force_en_us_locale to False by default, and enable it where necessary (?), don't really know where and for what it is used (is it really necessary), we only have commen:
        # Switch to English, because django-admin.py creates database content
        # like permissions, and those shouldn't contain any translations.
        # But only do this if we can assume we have a working settings file,
        # because django.utils.translation requires settings.
  • don't see any other/better better

comment:13 by Ramiro Morales, 12 years ago

Owner: changed from nobody to Ramiro Morales
Status: newassigned

comment:14 by Ramiro Morales, 12 years ago

Summary: Silent translation of all ./manage.py commands to en-us unexpected/undocumentedDon't force output of the 'shell' management command to en-us

I've opened #19730 to track the unfolding of the meaning of the can_import_settings option.

I'm also changing the title of this one from "Silent translation of all ./manage.py commands to en-us unexpected/undocumented" "Don't force output of the 'shell' management command to en-us" because the fact that by default commands use the en_US locale isn't undocumented (see "Management commands and locales" note in https://docs.djangoproject.com/en/1.4/howto/custom-management-commands/) and hence it shouldn't be unexpected.

Let's discuss in this ticket if it's possible to the 'shell' command to use the locale configured by the developer for the project if USE_I18N is active.

comment:15 by Ramiro Morales, 12 years ago

#19737 proposes deprecating the shell command. If accepted, that would turn this ticket irrelevant.

comment:16 by Claude Paroz, 12 years ago

I really think that this isn't about the shell command only. We should only switch to a specific locale when it is required by the command. I worked on a branch (proof of concept, unfinished) demonstrating this here: https://github.com/claudep/django/compare/simplify_commands
I'm not totally convinced about the decorator pattern used, but you get the idea.

comment:17 by Tim Graham, 11 years ago

Easy pickings: unset

comment:18 by Claude Paroz, 10 years ago

Summary: Don't force output of the 'shell' management command to en-usDon't deactivate translations by default in management commands

comment:19 by Claude Paroz, 7 years ago

Needs documentation: unset
Patch needs improvement: unset

I updated my branch and made it a PR.

comment:20 by Carlton Gibson, 7 years ago

Needs tests: set

comment:21 by Claude Paroz, 7 years ago

Patch needs improvement: set

comment:22 by Claude Paroz, 7 years ago

Needs tests: unset
Patch needs improvement: unset

comment:23 by Tim Graham, 7 years ago

Triage Stage: AcceptedReady for checkin
Type: BugCleanup/optimization

comment:24 by Claude Paroz <claude@…>, 7 years ago

Resolution: fixed
Status: assignedclosed

In d65b0f7:

Fixed #17379 -- Removed management commands deactivation of the locale.

Note: See TracTickets for help on using tickets.
Back to Top