Code

Opened 3 years ago

Closed 15 months ago

Last modified 9 months ago

#17008 closed New feature (fixed)

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

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

Download all attachments as: .zip

Change History (18)

Changed 3 years ago by airstrike

Adds --keep-pot option to makemessages.py

comment:1 Changed 3 years ago by aaugustin

  • Needs documentation set
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to 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).

Changed 3 years ago by airstrike

comment:2 Changed 3 years ago by airstrike

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 3 years ago by julien

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

comment:4 Changed 3 years ago by julien

  • Needs documentation unset
  • Patch needs improvement unset

comment:5 Changed 2 years ago by airstrike

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

Version 0, edited 2 years ago by airstrike (next)

Changed 2 years ago by airstrike

Changed 2 years ago by airstrike

docs/tests/patch in one single file

Changed 2 years ago by julien

comment:6 Changed 2 years ago by julien

  • 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 2 years ago by ramiro

Patch from Julien updated to changes that happened in makemessages code

comment:7 Changed 15 months ago by jezdez

  • Triage Stage changed from Accepted to Ready for checkin

comment:8 Changed 15 months ago by ramiro

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

Thanks Jannis for remembering about this ticket.

Last edited 15 months ago by ramiro (previous) (diff)

comment:9 Changed 15 months ago by Ramiro Morales <cramm0@…>

  • Resolution set to fixed
  • Status changed from new to closed

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 15 months ago by charettes

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

comment:11 Changed 9 months 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 9 months 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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.