Opened 12 years ago

Closed 11 years ago

Last modified 11 years ago

#17008 closed New feature (fixed)

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

Reported by: Andy Terra Owned by: Andy 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 Andy Terra 12 years ago.
Adds --keep-pot option to makemessages.py
0a4b228-patch.diff (5.2 KB ) - added by Andy Terra 12 years ago.
47b4795-tests-patch.diff (1.4 KB ) - added by Andy Terra 12 years ago.
final_patch.diff (6.6 KB ) - added by Andy Terra 12 years ago.
docs/tests/patch in one single file
17008.makemessages-keep-pot.diff (19.8 KB ) - added by Julien Phalip 12 years ago.
17008.makemessages-keep-pot.2.diff (13.2 KB ) - added by Ramiro Morales 12 years ago.
Patch from Julien updated to changes that happened in makemessages code

Download all attachments as: .zip

Change History (18)

by Andy Terra, 12 years ago

Attachment: patch.diff added

Adds --keep-pot option to makemessages.py

comment:1 by Aymeric Augustin, 12 years ago

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).

by Andy Terra, 12 years ago

Attachment: 0a4b228-patch.diff added

comment:2 by Andy Terra, 12 years ago

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 by Julien Phalip, 12 years ago

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

comment:4 by Julien Phalip, 12 years ago

Needs documentation: unset
Patch needs improvement: unset

comment:5 by Andy Terra, 12 years ago

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 12 years ago by Andy Terra (previous) (diff)

by Andy Terra, 12 years ago

Attachment: 47b4795-tests-patch.diff added

by Andy Terra, 12 years ago

Attachment: final_patch.diff added

docs/tests/patch in one single file

by Julien Phalip, 12 years ago

comment:6 by Julien Phalip, 12 years ago

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.

by Ramiro Morales, 12 years ago

Patch from Julien updated to changes that happened in makemessages code

comment:7 by Jannis Leidel, 11 years ago

Triage Stage: AcceptedReady for checkin

comment:8 by Ramiro Morales, 11 years ago

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

Thanks Jannis for remembering about this ticket.

Last edited 11 years ago by Ramiro Morales (previous) (diff)

comment:9 by Ramiro Morales <cramm0@…>, 11 years ago

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 by Simon Charette, 11 years ago

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

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

In 28d3b33c04cc2e2250059039c5ebbab97869d525:

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

comment:12 by Julien Phalip <jphalip@…>, 11 years ago

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