Opened 5 years ago

Closed 4 years ago

Last modified 3 years ago

#17008 closed New feature (fixed)

makemessages too eager to cleanup, makes it hard to debug errors

Reported by: André Terra Owned by: André Terra
Component: Core (Management commands) Version: 1.3
Severity: Normal Keywords: makemessages .pot .po gettext xgettext translation debug
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: yes UI/UX: no

Description

./manage.py makemessages always removes .pot files when finished, but sometimes this just makes debugging harder. I propose an option be added to allow for prevent deletion of these files, even when errors occur in the function.

The need for this option arose of an error which was, even when at verbose level 3, resulting in nearly useless debugging information when running ./manage.py makemessages --locale=pt_BR -a -d djangojs -v3:

processing language pt_BR
ignoring file .gitignore in .
ignoring file .project in .
ignoring file .pydevproject in .
ignoring file .urls.py.swp in .
ignoring file urls.py~ in .
processing file athena.js in .\static\js
(...)
processing file superfish.js in .\static\js
processing file grappelli.js in .\static\js\grappelli
processing file jquery.grp_collapsible.js in .\static\js\grappelli
processing file jquery.grp_collapsible_group.js in .\static\js\grappelli
processing file jquery.grp_inline.js in .\static\js\grappelli
processing file jquery.grp_related_fk.js in .\static\js\grappelli
processing file jquery.grp_related_generic.js in .\static\js\grappelli
processing file jquery.grp_related_m2m.js in .\static\js\grappelli
processing file jquery.grp_timepicker.js in .\static\js\grappelli
Error: errors happened while running msguniq
C:\projects\portal\hermes\locale\pt_BR\LC_MESSAGES\djangojs.pot:57: context separator <EOT> within string
C:\projects\portal\hermes\locale\pt_BR\LC_MESSAGES\djangojs.pot:58: context separator <EOT> within string
msguniq: found 2 fatal errors [1]

As you can see, it's hard to tell which of these files caused the EOT within string error, but by keeping djangojs.pot and then reading lines 57 and 58, I was able to deduct that athena.js was the file to blame

#: .\static\js\athena.js.c:30 .\static\js\athena.js.c:38
msgid "<EOT>" [2]
msgid_plural "<EOT>" [2]
msgstr[0] ""
msgstr[1] ""

[1] this line translated from Portuguese in this post to help understanding.

[2] these lines had a EOT character rather than <EOT> that can't be displayed here.

I've attached a simple patch which provides the user with a --keep-pot option to help in edge cases like this one, which simply skips the os.unlink(potfile) call.

Cheers!

Attachments (6)

patch.diff (4.9 KB) - added by André Terra 5 years ago.
Adds --keep-pot option to makemessages.py
0a4b228-patch.diff (5.2 KB) - added by André Terra 5 years ago.
47b4795-tests-patch.diff (1.4 KB) - added by André Terra 5 years ago.
final_patch.diff (6.6 KB) - added by André Terra 5 years ago.
docs/tests/patch in one single file
17008.makemessages-keep-pot.diff (19.8 KB) - added by Julien Phalip 5 years ago.
17008.makemessages-keep-pot.2.diff (13.2 KB) - added by Ramiro Morales 5 years ago.
Patch from Julien updated to changes that happened in makemessages code

Download all attachments as: .zip

Change History (18)

Changed 5 years ago by André Terra

Attachment: patch.diff added

Adds --keep-pot option to makemessages.py

comment:1 Changed 5 years ago by Aymeric Augustin

Needs documentation: set
Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

This looks useful enough to justify adding an option to makemessages. Error messages with references to non-existing files aren't useful!

The patch is a bit inconsistent: sometimes it uses unlink_potfile, sometimes if not keep_pot. I suggest removing the unlink_potfile function and always using a conditional — it's more explicit.

It also needs tests.

The options of makemessages are documented, you must include a doc patch too (see https://docs.djangoproject.com/en/dev/ref/django-admin/#makemessages).

NB: .pot files are removed since r15506 (although the patch was written much earlier).

Changed 5 years ago by André Terra

Attachment: 0a4b228-patch.diff added

comment:2 Changed 5 years ago by André Terra

I've improved the patch as requested, including the doc patch. I wasn't sure about the wording, and nobody replied in #django-dev, so I just wrote what I thought was relevant.

As for testing, I'm really not sure how to include this in tests/, as I couldn't spot where management commands tests are located. I have however tested it manually and it works as advertised even when errors in generating the .po occur.

If anyone could point me where in tests/ I should place the new test, I'd be glad to write it. Let me know if anything else is missing!

comment:3 Changed 5 years ago by Julien Phalip

Thanks, the patch looks pretty good. The test could live in tests/regressiontests/i18n/commands/extraction.py.

comment:4 Changed 5 years ago by Julien Phalip

Needs documentation: unset
Patch needs improvement: unset

comment:5 Changed 5 years ago by André Terra

I'm attaching a patch with the requested tests. All tests are passing here using Cygwin. I wasn't sure I needed to subclass the tearDown method to remove the .pot file, but I did so anyway.

foo@bar /c/temp/download/django-django-439ed79
$ tests/runtests.py -v3 --settings=test_sqlite i18n

(...)
----------------------------------------------------------------------
Ran 94 tests in 5.844s

OK

Let me know if anything else is missing! (hopefully not :P)

Cheers,

AT

Last edited 5 years ago by André Terra (previous) (diff)

Changed 5 years ago by André Terra

Attachment: 47b4795-tests-patch.diff added

Changed 5 years ago by André Terra

Attachment: final_patch.diff added

docs/tests/patch in one single file

Changed 5 years ago by Julien Phalip

comment:6 Changed 5 years ago by Julien Phalip

Patch needs improvement: set

I've updated the patch to current trunk and did a little pep8-cleanup.

It'd be nice to add more comprehensive tests. The main purpose of the --keep-pot option is for debugging in case the makemessages command (or more precisely the underlying calls to xgettext, msgmerge and msguniq) crashes, yet the patch currently only tests when everything works.

I'm not entirely sure how to write tests for this yet. Maybe that could be done by monkey-patching the _popen() function.

Changed 5 years ago by Ramiro Morales

Patch from Julien updated to changes that happened in makemessages code

comment:7 Changed 4 years ago by Jannis Leidel

Triage Stage: AcceptedReady for checkin

comment:8 Changed 4 years ago by Ramiro Morales

See https://github.com/ramiro/django/compare/17008-keep-pot for a refreshed patch.

Thanks Jannis for remebering about this ticket.

Version 0, edited 4 years ago by Ramiro Morales (next)

comment:9 Changed 4 years ago by Ramiro Morales <cramm0@…>

Resolution: fixed
Status: newclosed

In eee865257aaa9005947a7b4994c475c2ad59d698:

Fixed #17008 -- Added makemessages option to not remove .pot files.

Thanks airstrike for the report and initial patch, Julien for an
enhanced patch and Jannis for reviewing.

comment:10 Changed 4 years ago by Simon Charette

Maybe we could also add a release note about this new option.

comment:11 Changed 3 years ago by Julien Phalip <jphalip@…>

In 28d3b33c04cc2e2250059039c5ebbab97869d525:

Added a note to the 1.6 release about the new --keep-pot option for makemessages.
Refs #17008.

comment:12 Changed 3 years ago by Julien Phalip <jphalip@…>

In 59bf42f79e6f2fce2fc9128a86643cdb80b4b49a:

[1.6.x] Added a note to the 1.6 release about the new --keep-pot option for makemessages.
Refs #17008.
Backport of 28d3b33c04cc2 from master.

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