[Django] #29797: Allow the makemessages command to accept a directory to operate on like xgettext does
Django
xgettext does
-------------------------------------+-------------------------------------
Reporter: | Owner: nobody
terencehonles |
Type: New | Status: new
feature |
Component: Core | Version: 2.1
(Management commands) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 1
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
-------------------------------------+-------------------------------------
PR: https://github.com/django/django/pull/10422
Use case:
{{{
REPO_ROOT
|-- manage.py
|-- app/
| `-- ...
`-- .venv/
`-- ...
}}}
When ".venv" or another adjacent directory has a bad utf-8 marker or code
that would cause xgettext to fail the management command cannot succeed.
The workaround right now is to "cd" into the django_app and run
"../mange.py" so the current directory is now the django_app and
makemessages will properly exclude the other directories. Gettext has an
options "-D" or "--directory" which changes the directory to recurse from
the current directory to the specified directory. The patch linked adds
that option and walks that directory instead of the current directory from
the makemessages command.
--
Ticket URL: <https://code.djangoproject.com/ticket/29797>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
xgettext does
-------------------------------------+-------------------------------------
| terencehonles
Type: New feature | Status: assigned
Component: Core (Management | Version: 2.1
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => terencehonles
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/29797#comment:1>
Django
xgettext does
-------------------------------------+-------------------------------------
| terencehonles
Type: New feature | Status: assigned
Component: Core (Management | Version: 2.1
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Claude Paroz):
Mmmh, not convinced yet. You have at least four possible workarounds, the
one you mentioned in the ticket description, moving the .venv to a
superior level, using the `--ignore` flag or create your own makemessages
command subclass and complete the `xgettext_options` attribute.
I think this should be enough.
--
Ticket URL: <https://code.djangoproject.com/ticket/29797#comment:2>
Django
xgettext does
-------------------------------------+-------------------------------------
| terencehonles
Type: New feature | Status: closed
Component: Core (Management | Version: 2.1
commands) |
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => wontfix
Comment:
Yep, I agree with Claude. We closed #29734 for the same reason. (See
[https://docs.djangoproject.com/en/2.1/topics/i18n/translation
/#customizing-the-makemessages-command Customising the `makemessages`
command] to pass additional options to `xgettext`.)
--
Ticket URL: <https://code.djangoproject.com/ticket/29797#comment:3>
Django
xgettext does
-------------------------------------+-------------------------------------
| Honles
Type: New feature | Status: new
Component: Core (Management | Version: 2.1
commands) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: wontfix =>
Comment:
Replying to [comment:2 Claude Paroz]:
> Mmmh, not convinced yet. You have at least four possible workarounds,
the one you mentioned in the ticket description, moving the .venv to a
superior level, using the `--ignore` flag or create your own makemessages
command subclass and complete the `xgettext_options` attribute.
> I think this should be enough.
Adding to `xgettext_options` is not applicable because the list of files
to process is enumerated **by this command** and not xgettext which is
**why** I said this is allowing the command to emulate `xgettext`. Yes, I
could override the command and replace `find_files` to ignore the value
passed to it and hardcode the value I would pass for `-D` but having a
custom command that is making `find_files` ignore the only argument passed
to it seems silly and prone to error. The function already takes a
variable, why isn't it being used?
Using `--ignore` is heavy handed because it's only a
[https://github.com/django/django/blob/497e3942b53bc0d60aa2c688796c5c4bfc4b3df4/django/core/management/commands/makemessages.py#L461
basename check]
Moving the venv to a superior level is not acceptable because it is
already at the root of the repository.
Of course I can continue doing what I am doing, but I don't understand why
this **very simple** change which is **built in to xgettext** is not
acceptable. Please just look at the diff! It's 6 lines of code that
changed (4 of the 6 are just to take the command line argument) and makes
the function `find_files` actually useful. Why was it written to take an
argument but that argument always "."?
--
Ticket URL: <https://code.djangoproject.com/ticket/29797#comment:4>
Django
xgettext does
-------------------------------------+-------------------------------------
| Honles
Type: New feature | Status: new
Component: Core (Management | Version: 2.1
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Claude Paroz):
Replying to [comment:4 Terence Honles]:
> Using `--ignore` is heavy handed because it's only a
[https://github.com/django/django/blob/497e3942b53bc0d60aa2c688796c5c4bfc4b3df4/django/core/management/commands/makemessages.py#L461
basename check]
The ignore function just below the line you mention shows that it ignores
either on the basename or on the path. Please can you try that again?
--
Ticket URL: <https://code.djangoproject.com/ticket/29797#comment:5>
Django
xgettext does
-------------------------------------+-------------------------------------
| Honles
Type: New feature | Status: new
Component: Core (Management | Version: 2.1
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Terence Honles):
Replying to [comment:5 Claude Paroz]:
> Replying to [comment:4 Terence Honles]:
> > Using `--ignore` is heavy handed because it's only a
[https://github.com/django/django/blob/497e3942b53bc0d60aa2c688796c5c4bfc4b3df4/django/core/management/commands/makemessages.py#L461
basename check]
>
> The ignore function just below the line you mention shows that it
ignores either on the basename or on the path. Please can you try that
again?
You're right, I had familiarized myself more with `collectstatic` and it
handles things differently. Looking at the code more closely my issue with
`collectstatic` may have been the path prefix. I **am** able to ignore
directories properly with `--ignore`, however I still don't see what is
wrong with being able to set the `root` for `find_files`.
The code I am working with is very largely inherited and there are many
directories that need to be excluded so a single argument is preferred
over multiple that may need to be updated as files which break `xgettext`
are added via other tooling. It is nice to see I was incorrect about
`--ignore`, but for maintainability changing the current working directory
to exclude all the other files is still more bulletproof than having to
update an `--ignore` list. Even better would be defining the `root`
because it's more obvious to others than a `cd` dance.
--
Ticket URL: <https://code.djangoproject.com/ticket/29797#comment:6>
Django
xgettext does
-------------------------------------+-------------------------------------
| Honles
Type: New feature | Status: new
Component: Core (Management | Version: 2.1
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Claude Paroz):
Replying to [comment:6 Terence Honles]:
> I **am** able to ignore directories properly with `--ignore`, however I
still don't see what is wrong with being able to set the `root` for
`find_files`.
First I think that `makemessages` has already lots of options, and I don't
like seeing the list growing version after version.
But probably the most compelling reason is that for options that are meant
to always been set for some project (typically in your case), adding
options seems not the way to go in my opinion. Options are good for real
options, that is an optional choice the runner of the command may activate
or not depending on some preference. Typically, the locale to generate
messages for (with `--locale`). I'm aware that with this rule, several
existing options should go away, too!
So to come back to your case, I would be open to make the `find_files`
root more configurable, not through an option, but with a command
attribute, so you could add your own `makemessages` command subclass in
your project with something like this:
{{{
from django.core.management.commands.makemessages import Command as
MakeMessagesCommand
class Command(MakeMessagesCommand):
files_root = 'django_app'
}}}
Would you find that satisfactory for your use case?
--
Ticket URL: <https://code.djangoproject.com/ticket/29797#comment:7>
Django
xgettext does
-------------------------------------+-------------------------------------
| Honles
Type: New feature | Status: new
Component: Core (Management | Version: 2.1
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Terence Honles):
Replying to [comment:7 Claude Paroz]:
> Replying to [comment:6 Terence Honles]:
> > I **am** able to ignore directories properly with `--ignore`, however
I still don't see what is wrong with being able to set the `root` for
`find_files`.
>
> First I think that `makemessages` has already lots of options, and I
don't like seeing the list growing version after version.
> But probably the most compelling reason is that for options that are
meant to always been set for some project (typically in your case), adding
options seems not the way to go in my opinion. Options are good for real
options, that is an optional choice the runner of the command may activate
or not depending on some preference. Typically, the locale to generate
messages for (with `--locale`). I'm aware that with this rule, several
existing options should go away, too!
>
> So to come back to your case, I would be open to make the `find_files`
root more configurable, not through an option, but with a command
attribute, so you could add your own `makemessages` command subclass in
your project with something like this:
>
> {{{
> from django.core.management.commands.makemessages import Command as
MakeMessagesCommand
>
> class Command(MakeMessagesCommand):
> files_root = 'django_app'
> }}}
>
> Would you find that satisfactory for your use case?
Well I disagree about this not being a "real option" and I would
**prefer** not to have to create a custom command just to configure the
flag, but if it was easier to customize as you are suggesting I probably
would have just created the custom command rather than creating the PR (so
this is better than nothing). I still think extensibility should be
provided by options so things like "make" could hold the flags rather than
having a bunch of custom commands, but I understand in world without build
tools you would want to make sure every user used the same options.
Obviously this is a discussion about where your configuration lives. I
would prefer it to be in my build tooling and others may prefer to create
commands so it's "build in" to the `manage.py <subcommand>`. For me the
preference is my config will live in one file rather than distributed
across the filesystem.
I obviously have stated my preference, but thank you for discussing this
with me.
--
Ticket URL: <https://code.djangoproject.com/ticket/29797#comment:8>
Django
xgettext does
-------------------------------------+-------------------------------------
| Honles
Component: Core (Management | Version: 2.1
commands) |
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => wontfix
Comment:
Thanks for the thoughtful discussion all.
I'm going to close again. I think Claude's points cover what we'll ship in
core.
It'd be a bit more work for your use-case but I expect writing a wrapper
script to take the options you want would not be infeasible.
Sorry if that's not perfect for you Terence.
--
Ticket URL: <https://code.djangoproject.com/ticket/29797#comment:9>