Opened 9 years ago

Closed 8 years ago

Last modified 5 years ago

#5463 closed (fixed)

make-messages.py process only .html (not .tpl etc)

Reported by: beer <antiwin@…> Owned by: Jannis Leidel
Component: Internationalization Version: master
Severity: Keywords:
Cc: John Shaffer, matthijs@…, jannis@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

there should be in settings ie:
I18N_TEMPLATES_EXTENSIONS = ('.html','.tpl')
and default ('.html') for backward compatibility

Attachments (6)

5463.diff (2.6 KB) - added by beer 9 years ago.
make-messages.1.diff (7.2 KB) - added by Jannis Leidel 8 years ago.
This is a patch that depends on the changes made in patch 6 of ticket #5522 and adds a --extension option to the new djangoadmin.py makemessages tool, as discussed with jacob. ".html" stays the default. fully backwards-compatible and documented.
t5463-r7720.diff (8.9 KB) - added by Ramiro Morales 8 years ago.
t5463-r7844.diff (7.2 KB) - added by Jannis Leidel 8 years ago.
Updated to trunk after merge of #5522, added section to i18n.txt
t5463-r8150.diff (7.1 KB) - added by Jannis Leidel 8 years ago.
Fixed problem as described by Malcolm.
t5463-r8150.2.diff (7.1 KB) - added by Jannis Leidel 8 years ago.
Fixed a bug when using the djangojs domain.

Download all attachments as: .zip

Change History (33)

comment:1 Changed 9 years ago by beer <antiwin@…>

Component: TranslationsInternationalization
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

comment:2 Changed 9 years ago by beer

Keywords: sprintsept14 added

comment:3 Changed 9 years ago by Julian Bez

Triage Stage: UnreviewedDesign decision needed

comment:4 Changed 9 years ago by Fredrik Lundh <fredrik@…>

A little nitpicking:

  • the imp.load_module stuff looks like overkill; I'd use
try:
    settings_dict = {}
    execfile("settings.py", settings_dict) 
    I18N_TEMPLATE_EXTENSIONS = settings_dict["I18N_TEMPLATE_EXTENSIONS"]
except (IOError, KeyError): 
    I18N_TEMPLATE_EXTENSIONS = settings.I18N_TEMPLATE_EXTENSIONS 

(I have no opinion on whether a check for a local file is a good idea or not)

  • Python standard is to include the leading period in extensions; e.g. (".html", ".xml") etc. Not sure what the Django standard is; couldn't find any prior art here. Anyway, if you switch to Python style, you can use "name, ext = os.path.splitext(filename)" to do the split.

comment:5 Changed 9 years ago by Malcolm Tredinnick

I'm not completely convinced a setting is the right idea here, but we should allow arbitrary extensions or something similar, you're right.

Still needs some design work (meaning "some more thinking"), but it's a good idea.

Changed 9 years ago by beer

Attachment: 5463.diff added

comment:6 Changed 9 years ago by beer

new version: no settings; add some ordinary template extensions as default (html, tpl, xml, txt); add argument -e[list of templates] ie.: -e .asd .zxc

comment:7 Changed 9 years ago by beer

Has patch: set
Triage Stage: Design decision neededReady for checkin

comment:8 Changed 9 years ago by James Bennett

Triage Stage: Ready for checkinAccepted

Please don't mark tickets "ready for checkin". Our triagers and core team make that call.

comment:9 Changed 9 years ago by Malcolm Tredinnick

The default should remain as it is (.html), rather than scanning lots of potentially unneeded files. The rest of the patch is fine.

No need to upload a new patch.. I'll fix it when I check it in.

comment:10 Changed 9 years ago by John Shaffer

Cc: John Shaffer added

comment:11 Changed 8 years ago by Matthijs Kooijman <matthijs@…>

Cc: matthijs@… added

comment:12 Changed 8 years ago by Jannis Leidel

Cc: jannis@… added

comment:13 Changed 8 years ago by Jacob

Resolution: duplicate
Status: newclosed

#7180 was a duplicate.

comment:14 Changed 8 years ago by Jacob

Resolution: duplicate
Status: closedreopened

Woops, the other one's a dup, not this one.

Changed 8 years ago by Jannis Leidel

Attachment: make-messages.1.diff added

This is a patch that depends on the changes made in patch 6 of ticket #5522 and adds a --extension option to the new djangoadmin.py makemessages tool, as discussed with jacob. ".html" stays the default. fully backwards-compatible and documented.

comment:15 Changed 8 years ago by Jannis Leidel

Owner: changed from nobody to Jannis Leidel
Status: reopenednew

comment:16 Changed 8 years ago by Jannis Leidel

Status: newassigned

comment:17 Changed 8 years ago by Ramiro Morales

#7520 was a duplicate. One valid point it makes is about the need to also update docs/i18n.txt

comment:18 in reply to:  17 ; Changed 8 years ago by Jannis Leidel

Replying to ramiro:

#7520 was a duplicate. One valid point it makes is about the need to also update docs/i18n.txt

I don't see any valid point there, there *is* documentation in the latest patch here which needs the changes in #5522.

comment:19 in reply to:  18 Changed 8 years ago by Ramiro Morales

Replying to jezdez:

I don't see any valid point there, there *is* documentation in the latest patch here which needs the changes in #5522.

I know the patch includes updates to the documentation, but I was specifically talking about i18n.txt. Perhaps adding a note to the Message files section about what file extensions does make-messages.py processes by default and how to change it. Let me know what do you think, I could try to come up with a draft.

Meanwhile, find attached a revised (up to trunk as of r7720) version of the patch because make-messages.1.diff has some problems:

  • make-messages.py: A stray (debug?) print extensions
  • make-messages.py: wrapping/indenting of -a and -e command line switches description is wrong when using -h
  • Usage of mutable sequences as default values of functions parameters
  • Inconsistent usage of organize_extensions/handle_extensions function name (django-admin.py makemessages is currently broken because of this)

Changed 8 years ago by Ramiro Morales

Attachment: t5463-r7720.diff added

comment:20 Changed 8 years ago by Jannis Leidel

milestone: 1.0 alpha

comment:21 Changed 8 years ago by Marc Garcia

Keywords: sprintsept14 removed
milestone: 1.0 alphapost-1.0

According to ticket organization defined in http://code.djangoproject.com/wiki/VersionOneRoadmap#how-you-can-help 1.0 alpha tickets should be just features in the Must have (http://code.djangoproject.com/wiki/VersionOneRoadmap#must-have-features) list.

Change to 1.0 beta if you can make this feature be added to May be features (http://code.djangoproject.com/wiki/VersionOneRoadmap#maybe-features).

comment:22 Changed 8 years ago by Jannis Leidel

milestone: post-1.01.0 beta

Thanks but this is connected to #5522 and will be in Django 1.0 (as discussed with Jacob in #django-dev).

Changed 8 years ago by Jannis Leidel

Attachment: t5463-r7844.diff added

Updated to trunk after merge of #5522, added section to i18n.txt

comment:23 Changed 8 years ago by Malcolm Tredinnick

Patch needs improvement: set

I read through the latest patch. I have a few comments that should probably be looked at before we commit it. Mostly minor things, but there are enough of them that I'm going to bounce it back, rather than tweak them all myself.

  1. Passing in the default value to handle_extensions() doesn't look right. Why not just make the extensions parameter take ('html,) as a default? Basically, there's no reason that default will ever not be '.html', so it shouldn't be a parameter.
  2. Converting the extension to lower-case (line 30) is wrong. Most sensible file systems are case sensitive, so .HTML and .html are different and I'll specify the one I mean on the command line. Most people won't have both, but forcing things to lower-case will cause mismatches in some situations.
  3. Command line parameters can't contain spaces (otherwise there's no way to tell them apart from the next parameter, for example), so you just use .split(',') on line 30. Also, probably prefer some_list.extend(...) over some_list += ...., since it's clearer that you're updating in place and not using some O(n2) copying algorithm.
  4. Line 38 is a real masterpiece of data structure changes: list -> set -> list again, all in one line. :-) You can probably just return a set there, instead of going back to a list. Sets are iterable and that's all you're using it for. This is both a bit shorter and slightly more efficient (you don't go from a set to a list on line 38 and, later one, x in extensions is faster, because "in" for sets is O(1), rather than the O(n) for lists).
  5. Still on line 38, the comparison in the list comprehension is written in a way that looks like it might be a bug (it isn't, but rewrite it to avoid the confusion). Drop the parentheses around the '.py bit, since they're unnecessary. And change not x == '.py' to x != '.py' while you're in there. It's a bit easier to read.
  6. This custom extension stuff really only makes sense for the "django" domain (not "djangojs"), so you might want to make that clear in the documentation. I fear that everybody forgets about poor "djangojs", since "django" is the default domain. But we can probably gamble that all Javascript files end in .js, so you don't have to do anything there (might be worth raising an error is -e is passed in for that case, though).

Once all that is done, I think we're ready to go here.

comment:24 in reply to:  23 Changed 8 years ago by Jannis Leidel

Replying to mtredinnick:

  1. Passing in the default value to handle_extensions() doesn't look right. Why not just make the extensions parameter take ('html,) as a default? Basically, there's no reason that default will ever not be '.html', so it shouldn't be a parameter.

Fixed.

  1. Converting the extension to lower-case (line 30) is wrong. Most sensible file systems are case sensitive, so .HTML and .html are different and I'll specify the one I mean on the command line. Most people won't have both, but forcing things to lower-case will cause mismatches in some situations.

Fixed.

  1. Command line parameters can't contain spaces (otherwise there's no way to tell them apart from the next parameter, for example), so you just use .split(',') on line 30. Also, probably prefer some_list.extend(...) over some_list += ...., since it's clearer that you're updating in place and not using some O(n2) copying algorithm.

Oh they can contain spaces, by using something like django-admin.py makemessages -e "tpl, html, xml" . I know this is an edge case, but is replace(' ', '') really too expensive at the point? (I left it in the current patch)

  1. Line 38 is a real masterpiece of data structure changes: list -> set -> list again, all in one line. :-) You can probably just return a set there, instead of going back to a list. Sets are iterable and that's all you're using it for. This is both a bit shorter and slightly more efficient (you don't go from a set to a list on line 38 and, later one, x in extensions is faster, because "in" for sets is O(1), rather than the O(n) for lists).

Hehe, fixed.

  1. Still on line 38, the comparison in the list comprehension is written in a way that looks like it might be a bug (it isn't, but rewrite it to avoid the confusion). Drop the parentheses around the '.py bit, since they're unnecessary. And change not x == '.py' to x != '.py' while you're in there. It's a bit easier to read.

Fixed.

  1. This custom extension stuff really only makes sense for the "django" domain (not "djangojs"), so you might want to make that clear in the documentation. I fear that everybody forgets about poor "djangojs", since "django" is the default domain. But we can probably gamble that all Javascript files end in .js, so you don't have to do anything there (might be worth raising an error is -e is passed in for that case, though).

Fixed.

Changed 8 years ago by Jannis Leidel

Attachment: t5463-r8150.diff added

Fixed problem as described by Malcolm.

Changed 8 years ago by Jannis Leidel

Attachment: t5463-r8150.2.diff added

Fixed a bug when using the djangojs domain.

comment:25 Changed 8 years ago by John Shaffer

Patch needs improvement: unset

comment:26 Changed 8 years ago by Malcolm Tredinnick

Resolution: fixed
Status: assignedclosed

(In [8234]) Fixed #5463 -- Allow alternate file extensions on files that are translated.
Patch from Jannis Leidel.

comment:27 Changed 5 years ago by Jacob

milestone: 1.0 beta

Milestone 1.0 beta deleted

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