Opened 17 years ago

Closed 16 years ago

Last modified 13 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: dev
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: no UI/UX: no

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 17 years ago.
make-messages.1.diff (7.2 KB ) - added by Jannis Leidel 16 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 16 years ago.
t5463-r7844.diff (7.2 KB ) - added by Jannis Leidel 16 years ago.
Updated to trunk after merge of #5522, added section to i18n.txt
t5463-r8150.diff (7.1 KB ) - added by Jannis Leidel 16 years ago.
Fixed problem as described by Malcolm.
t5463-r8150.2.diff (7.1 KB ) - added by Jannis Leidel 16 years ago.
Fixed a bug when using the djangojs domain.

Download all attachments as: .zip

Change History (33)

comment:1 by beer <antiwin@…>, 17 years ago

Component: TranslationsInternationalization

comment:2 by beer, 17 years ago

Keywords: sprintsept14 added

comment:3 by Julian Bez, 17 years ago

Triage Stage: UnreviewedDesign decision needed

comment:4 by Fredrik Lundh <fredrik@…>, 17 years ago

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 by Malcolm Tredinnick, 17 years ago

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.

by beer, 17 years ago

Attachment: 5463.diff added

comment:6 by beer, 17 years ago

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 by beer, 17 years ago

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

comment:8 by James Bennett, 17 years ago

Triage Stage: Ready for checkinAccepted

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

comment:9 by Malcolm Tredinnick, 17 years ago

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 by John Shaffer, 17 years ago

Cc: John Shaffer added

comment:11 by Matthijs Kooijman <matthijs@…>, 17 years ago

Cc: matthijs@… added

comment:12 by Jannis Leidel, 16 years ago

Cc: jannis@… added

comment:13 by Jacob, 16 years ago

Resolution: duplicate
Status: newclosed

#7180 was a duplicate.

comment:14 by Jacob, 16 years ago

Resolution: duplicate
Status: closedreopened

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

by Jannis Leidel, 16 years ago

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 by Jannis Leidel, 16 years ago

Owner: changed from nobody to Jannis Leidel
Status: reopenednew

comment:16 by Jannis Leidel, 16 years ago

Status: newassigned

comment:17 by Ramiro Morales, 16 years ago

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

in reply to:  17 ; comment:18 by Jannis Leidel, 16 years ago

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.

in reply to:  18 comment:19 by Ramiro Morales, 16 years ago

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)

by Ramiro Morales, 16 years ago

Attachment: t5463-r7720.diff added

comment:20 by Jannis Leidel, 16 years ago

milestone: 1.0 alpha

comment:21 by Marc Garcia, 16 years ago

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 by Jannis Leidel, 16 years ago

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

by Jannis Leidel, 16 years ago

Attachment: t5463-r7844.diff added

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

comment:23 by Malcolm Tredinnick, 16 years ago

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.

in reply to:  23 comment:24 by Jannis Leidel, 16 years ago

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.

by Jannis Leidel, 16 years ago

Attachment: t5463-r8150.diff added

Fixed problem as described by Malcolm.

by Jannis Leidel, 16 years ago

Attachment: t5463-r8150.2.diff added

Fixed a bug when using the djangojs domain.

comment:25 by John Shaffer, 16 years ago

Patch needs improvement: unset

comment:26 by Malcolm Tredinnick, 16 years ago

Resolution: fixed
Status: assignedclosed

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

comment:27 by Jacob, 13 years ago

milestone: 1.0 beta

Milestone 1.0 beta deleted

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