#5463 closed (fixed)
make-messages.py process only .html (not .tpl etc)
Reported by: | 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)
Change History (33)
comment:1 by , 17 years ago
Component: | Translations → Internationalization |
---|
comment:2 by , 17 years ago
Keywords: | sprintsept14 added |
---|
comment:3 by , 17 years ago
Triage Stage: | Unreviewed → Design decision needed |
---|
comment:4 by , 17 years ago
comment:5 by , 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 , 17 years ago
comment:6 by , 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 , 17 years ago
Has patch: | set |
---|---|
Triage Stage: | Design decision needed → Ready for checkin |
comment:8 by , 17 years ago
Triage Stage: | Ready for checkin → Accepted |
---|
Please don't mark tickets "ready for checkin". Our triagers and core team make that call.
comment:9 by , 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 , 17 years ago
Cc: | added |
---|
comment:11 by , 17 years ago
Cc: | added |
---|
comment:12 by , 16 years ago
Cc: | added |
---|
comment:14 by , 16 years ago
Resolution: | duplicate |
---|---|
Status: | closed → reopened |
Woops, the other one's a dup, not this one.
by , 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 , 16 years ago
Owner: | changed from | to
---|---|
Status: | reopened → new |
comment:16 by , 16 years ago
Status: | new → assigned |
---|
follow-up: 18 comment:17 by , 16 years ago
#7520 was a duplicate. One valid point it makes is about the need to also update docs/i18n.txt
follow-up: 19 comment:18 by , 16 years ago
comment:19 by , 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 , 16 years ago
Attachment: | t5463-r7720.diff added |
---|
comment:20 by , 16 years ago
milestone: | → 1.0 alpha |
---|
comment:21 by , 16 years ago
Keywords: | sprintsept14 removed |
---|---|
milestone: | 1.0 alpha → 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 by , 16 years ago
milestone: | post-1.0 → 1.0 beta |
---|
Thanks but this is connected to #5522 and will be in Django 1.0 (as discussed with Jacob in #django-dev).
by , 16 years ago
Attachment: | t5463-r7844.diff added |
---|
Updated to trunk after merge of #5522, added section to i18n.txt
follow-up: 24 comment:23 by , 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.
- Passing in the default value to
handle_extensions()
doesn't look right. Why not just make theextensions
parameter take('html,)
as a default? Basically, there's no reason thatdefault
will ever not be'.html'
, so it shouldn't be a parameter. - 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. - 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 prefersome_list.extend(...)
oversome_list += ....
, since it's clearer that you're updating in place and not using some O(n2) copying algorithm. - 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). - 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 changenot x == '.py'
tox != '.py'
while you're in there. It's a bit easier to read. - 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 by , 16 years ago
Replying to mtredinnick:
- Passing in the default value to
handle_extensions()
doesn't look right. Why not just make theextensions
parameter take('html,)
as a default? Basically, there's no reason thatdefault
will ever not be'.html'
, so it shouldn't be a parameter.
Fixed.
- 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.
- 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 prefersome_list.extend(...)
oversome_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)
- 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.
- 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 changenot x == '.py'
tox != '.py'
while you're in there. It's a bit easier to read.
Fixed.
- 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.
comment:25 by , 16 years ago
Patch needs improvement: | unset |
---|
comment:26 by , 16 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
A little nitpicking:
(I have no opinion on whether a check for a local file is a good idea or not)