#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)
Change History (18)
by , 13 years ago
Attachment: | patch.diff added |
---|
comment:1 by , 13 years ago
Needs documentation: | set |
---|---|
Needs tests: | set |
Patch needs improvement: | set |
Triage Stage: | Unreviewed → Accepted |
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 , 13 years ago
Attachment: | 0a4b228-patch.diff added |
---|
comment:2 by , 13 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 , 13 years ago
Thanks, the patch looks pretty good. The test could live in tests/regressiontests/i18n/commands/extraction.py
.
comment:4 by , 13 years ago
Needs documentation: | unset |
---|---|
Patch needs improvement: | unset |
comment:5 by , 13 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
by , 13 years ago
Attachment: | 47b4795-tests-patch.diff added |
---|
by , 13 years ago
Attachment: | 17008.makemessages-keep-pot.diff added |
---|
comment:6 by , 13 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 , 13 years ago
Attachment: | 17008.makemessages-keep-pot.2.diff added |
---|
Patch from Julien updated to changes that happened in makemessages code
comment:7 by , 12 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:8 by , 12 years ago
See https://github.com/ramiro/django/compare/17008-keep-pot for a refreshed patch.
Thanks Jannis for remembering about this ticket.
comment:9 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Adds --keep-pot option to makemessages.py