Issue 5694103719247872: Issue 2133 - Allow to specify default translation inline in pages rather than in a separate file

Issue 5694103719247872: Issue 2133 - Allow to specify default translation inline in pages rather than in a separate file (Closed)

Created:
March 12, 2015, 7:34 p.m. by Wladimir Palant

Modified:
March 12, 2015, 9:15 p.m.

Reviewers:
Sebastian Noack

CC:
kzar

Visibility:
Public.

Description

Issue 2133 - Allow to specify default translation inline in pages rather than in a separate file

Patch Set 2 : Addressed comments #

Created: March 12, 2015, 8:56 p.m.

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+129 lines, -51 lines)			Patch
M	cms/bin/generate_static_pages.py	View		1 chunk	+3 lines, -1 line	0 comments	Download
M	cms/converters.py	View	1	6 chunks	+104 lines, -40 lines	0 comments	Download
M	cms/sources.py	View		2 chunks	+10 lines, -3 lines	0 comments	Download
M	cms/utils.py	View	1	2 chunks	+12 lines, -7 lines	0 comments	Download

Messages

Total messages: 5

Expand All Messages | Collapse All Messages

Sebastian Noack

http://codereview.adblockplus.org/5694103719247872/diff/5629499534213120/cms/converters.py File cms/converters.py (right): http://codereview.adblockplus.org/5694103719247872/diff/5629499534213120/cms/converters.py#newcode45 cms/converters.py:45: class AttributeParser(HTMLParser.HTMLParser): That makes actually sense, using a proper ...

March 12, 2015, 8:33 p.m. (2015-03-12 20:33:46 UTC) #2

Wladimir Palant

March 12, 2015, 8:57 p.m. (2015-03-12 20:57:02 UTC) #3

http://codereview.adblockplus.org/5694103719247872/diff/5629499534213120/cms/...
File cms/converters.py (right):

http://codereview.adblockplus.org/5694103719247872/diff/5629499534213120/cms/...
cms/converters.py:45: class AttributeParser(HTMLParser.HTMLParser):
On 2015/03/12 20:33:46, Sebastian Noack wrote:
> That makes actually sense, using a proper parser instead a regex. I'm just
> wondring whether it explodes if we get not 100% valid HTML, or whether this is
a
> case we have to bother about here.

I actually tested this - invalid HTML will be normalized, which is a nice
side-effect.

http://codereview.adblockplus.org/5694103719247872/diff/5629499534213120/cms/...
cms/converters.py:54: self._string = ""
On 2015/03/12 20:33:46, Sebastian Noack wrote:
> I'd rather use a list here, joining it when done. I know performance doesn't
> matter here, but Python folks just consider this kind of string concatenation
> ugly. ;)

Done.

http://codereview.adblockplus.org/5694103719247872/diff/5629499534213120/cms/...
File cms/sources.py (right):

http://codereview.adblockplus.org/5694103719247872/diff/5629499534213120/cms/...
cms/sources.py:79: from .converters import converters
On 2015/03/12 20:33:46, Sebastian Noack wrote:
> From PEP-8: "Relative imports for intra-package imports are highly
discouraged.
> Always use the absolute package path for all imports. Even now that PEP 328
[7]
> is fully implemented in Python 2.5, its style of explicit relative imports is
> actively discouraged; absolute imports are more portable and usually more
> readable."

Relative imports actually made moving this out of sitescripts simpler ;)

Either way, I know what you think about relative imports. However, they are used
consistently across the CMS. So I'd rather keep things consistent and replace
relative imports in a separate change.

http://codereview.adblockplus.org/5694103719247872/diff/5629499534213120/cms/...
cms/sources.py:82: for format in converters.iterkeys()
On 2015/03/12 20:33:46, Sebastian Noack wrote:
> Nit: Do you know that you can lazily iterate over the dictionary itself,
> yielding its keys? So there is no need to call .iterkeys() here.

Yes, I know that. However, I prefer to keep things obvious.

http://codereview.adblockplus.org/5694103719247872/diff/5629499534213120/cms/...
File cms/utils.py (right):

http://codereview.adblockplus.org/5694103719247872/diff/5629499534213120/cms/...
cms/utils.py:60: params["available_locales"].sort()
On 2015/03/12 20:33:46, Sebastian Noack wrote:
> Four inline lookups for "available_locales" and three for "defaultloacle",
that
> calls for local variables.

Not sure it really makes the code better here but done.

Sebastian Noack

LGTM. The one remaining nit isn't worth arguing. http://codereview.adblockplus.org/5694103719247872/diff/5629499534213120/cms/sources.py File cms/sources.py (right): http://codereview.adblockplus.org/5694103719247872/diff/5629499534213120/cms/sources.py#newcode79 cms/sources.py:79: from ...

March 12, 2015, 9:08 p.m. (2015-03-12 21:08:18 UTC) #4

LGTM. The one remaining nit isn't worth arguing.

http://codereview.adblockplus.org/5694103719247872/diff/5629499534213120/cms/...
File cms/sources.py (right):

http://codereview.adblockplus.org/5694103719247872/diff/5629499534213120/cms/...
cms/sources.py:79: from .converters import converters
On 2015/03/12 20:57:02, Wladimir Palant wrote:
> On 2015/03/12 20:33:46, Sebastian Noack wrote:
> > From PEP-8: "Relative imports for intra-package imports are highly
> discouraged.
> > Always use the absolute package path for all imports. Even now that PEP 328
> [7]
> > is fully implemented in Python 2.5, its style of explicit relative imports
is
> > actively discouraged; absolute imports are more portable and usually more
> > readable."
> 
> Relative imports actually made moving this out of sitescripts simpler ;)
> 
> Either way, I know what you think about relative imports. However, they are
used
> consistently across the CMS. So I'd rather keep things consistent and replace
> relative imports in a separate change.

Fair enough. I'm looking forward that change. :)

http://codereview.adblockplus.org/5694103719247872/diff/5629499534213120/cms/...
cms/sources.py:82: for format in converters.iterkeys()
On 2015/03/12 20:57:02, Wladimir Palant wrote:
> On 2015/03/12 20:33:46, Sebastian Noack wrote:
> > Nit: Do you know that you can lazily iterate over the dictionary itself,
> > yielding its keys? So there is no need to call .iterkeys() here.
> 
> Yes, I know that. However, I prefer to keep things obvious.

So you would also use ..

for (let key of Object.keys(o))

.. instead ..

for (let key in o)

.. in JavaScript? ;)

http://codereview.adblockplus.org/5694103719247872/diff/5629499534213120/cms/...
File cms/utils.py (right):

http://codereview.adblockplus.org/5694103719247872/diff/5629499534213120/cms/...
cms/utils.py:60: params["available_locales"].sort()
On 2015/03/12 20:57:02, Wladimir Palant wrote:
> On 2015/03/12 20:33:46, Sebastian Noack wrote:
> > Four inline lookups for "available_locales" and three for "defaultloacle",
> that
> > calls for local variables.
> 
> Not sure it really makes the code better here but done.

Much better. Thanks.

Wladimir Palant

March 12, 2015, 9:15 p.m. (2015-03-12 21:15:27 UTC) #5

http://codereview.adblockplus.org/5694103719247872/diff/5629499534213120/cms/...
File cms/sources.py (right):

http://codereview.adblockplus.org/5694103719247872/diff/5629499534213120/cms/...
cms/sources.py:82: for format in converters.iterkeys()
On 2015/03/12 21:08:19, Sebastian Noack wrote:
> So you would also use ..
> 
> for (let key of Object.keys(o))
> 
> .. instead ..
> 
> for (let key in o)
> 
> .. in JavaScript? ;)

These constructs aren't actually equivalent. Besides, the for..in loop is meant
specifically to iterate over properties of objects, it won't do anything beyond
that. A Python loop on the other hand can iterate over all kinds of things, you
actually have to know what kind of object you have there. It's the same with the
for..of loop in JavaScript and I'm not sure I'd approve any non-obvious use of
it (meaning: not an array).

Expand All Messages | Collapse All Messages