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

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.

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.

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

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

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.

#7180 was a duplicate.

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

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.

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

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.

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)

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

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

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

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(n^{2) 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.

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.

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.

Fixed.

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(n^{2) 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)

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

Hehe, fixed.

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.

Fixed.

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

Fixed.

Fixed problem as described by Malcolm.

Fixed a bug when using the djangojs domain.

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