Opened 7 years ago

Closed 7 years ago

Last modified 3 years ago

#5463 closed (fixed)

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

Reported by: beer <antiwin@…> Owned by: jezdez
Component: Internationalization Version: master
Severity: Keywords:
Cc: jshaffer, 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 7 years ago.
make-messages.1.diff (7.2 KB) - added by jezdez 7 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 7 years ago.
t5463-r7844.diff (7.2 KB) - added by jezdez 7 years ago.
Updated to trunk after merge of #5522, added section to i18n.txt
t5463-r8150.diff (7.1 KB) - added by jezdez 7 years ago.
Fixed problem as described by Malcolm.
t5463-r8150.2.diff (7.1 KB) - added by jezdez 7 years ago.
Fixed a bug when using the djangojs domain.

Download all attachments as: .zip

Change History (33)

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

  • Component changed from Translations to Internationalization
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 7 years ago by beer

  • Keywords sprintsept14 added

comment:3 Changed 7 years ago by webjunkie

  • Triage Stage changed from Unreviewed to Design decision needed

comment:4 Changed 7 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 7 years ago by mtredinnick

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

comment:6 Changed 7 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 7 years ago by beer

  • Has patch set
  • Triage Stage changed from Design decision needed to Ready for checkin

comment:8 Changed 7 years ago by ubernostrum

  • Triage Stage changed from Ready for checkin to Accepted

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

comment:9 Changed 7 years ago by mtredinnick

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 7 years ago by jshaffer

  • Cc jshaffer added

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

  • Cc matthijs@… added

comment:12 Changed 7 years ago by jezdez

  • Cc jannis@… added

comment:13 Changed 7 years ago by jacob

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

#7180 was a duplicate.

comment:14 Changed 7 years ago by jacob

  • Resolution duplicate deleted
  • Status changed from closed to reopened

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

Changed 7 years ago by jezdez

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 7 years ago by jezdez

  • Owner changed from nobody to jezdez
  • Status changed from reopened to new

comment:16 Changed 7 years ago by jezdez

  • Status changed from new to assigned

comment:17 follow-up: Changed 7 years ago by ramiro

#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 ; follow-up: Changed 7 years ago by jezdez

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

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

comment:20 Changed 7 years ago by jezdez

  • milestone set to 1.0 alpha

comment:21 Changed 7 years ago by garcia_marc

  • Keywords sprintsept14 removed
  • milestone changed from 1.0 alpha to post-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 7 years ago by jezdez

  • milestone changed from post-1.0 to 1.0 beta

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

Changed 7 years ago by jezdez

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

comment:23 follow-up: Changed 7 years ago by mtredinnick

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

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 7 years ago by jezdez

Fixed problem as described by Malcolm.

Changed 7 years ago by jezdez

Fixed a bug when using the djangojs domain.

comment:25 Changed 7 years ago by jshaffer

  • Patch needs improvement unset

comment:26 Changed 7 years ago by mtredinnick

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

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

comment:27 Changed 3 years ago by jacob

  • milestone 1.0 beta deleted

Milestone 1.0 beta deleted

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