Fix for passing interpolation mappings in translator calls. #5

Merged
merged 2 commits into from Feb 3, 2012

Conversation

Projects
None yet
3 participants
@dokai
Contributor

dokai commented Jan 19, 2012

Previously it was possible to provide an interpolation mapping only as part of
a translation string. Providing it later during translation would be ignored.

This change allows you to provide the mapping as part of the translator call,
e.g.

translator(tstring, mapping={...})

if the tstring does not contain a mapping itself, the mapping given in the
translator() call will be used as is. Otherwise, the previously given mapping
will be updated with the new values allowing partial overriding of an existing
mapping.

Fixed an issue with passing an interpolation mapping in translator.
Previously it was possible to provide an interpolation mapping only as part of
a translation string. Providing it later during translation would be ignored.

This change allows you to provide the mapping as part of the translator call,
e.g.

  translator(tstring, mapping={...})

if the tstring does not contain a mapping itself, the mapping given in the
translator() call will be used as is. Otherwise, the previously given mapping
will be updated with the new values allowing partial overriding of an existing
mapping.
+ if tstring.mapping is None:
+ tstring.mapping = mapping
+ elif mapping is not None:
+ tstring.mapping.update(mapping)

This comment has been minimized.

Show comment Hide comment
@mgedmin

mgedmin Jan 19, 2012

I wonder if you should add an explicit unit test for the mapping.update(). Now you can replace this line with tstring.mapping = mapping and your tests will still pass.

Consider

tstring = TranslationString('${greeting}, ${world}', mapping=dict(greeting='Hello', world='world'))
print translator(tstring, mapping=dict(greeting='Goodbye'))

Is this a sane allowed use case that should be supported?

@mgedmin

mgedmin Jan 19, 2012

I wonder if you should add an explicit unit test for the mapping.update(). Now you can replace this line with tstring.mapping = mapping and your tests will still pass.

Consider

tstring = TranslationString('${greeting}, ${world}', mapping=dict(greeting='Hello', world='world'))
print translator(tstring, mapping=dict(greeting='Goodbye'))

Is this a sane allowed use case that should be supported?

This comment has been minimized.

Show comment Hide comment
@dokai

dokai Jan 19, 2012

Contributor

I agree that an explicit unit test would better communicate the intended behavior. I did originally mean it to have the .update() semantics so that you would "inherit" the existing mapping (if available) and then be able to (possibly partially) override it with the translator() level mapping.

The API docs state that

"""The optional mapping argument can specify or override the tstring interpolation mapping"""

and reading that again does make me think that the "override" part would be better implemented as "replaces the original mapping" instead of "updates the original mapping". I don't really have a strong opinion about this since in most cases I believe you would end up providing a complete mapping in the translator() call anyway.

I'd be happy to change the patch to just overriding tstring.mapping. Opinions?

@dokai

dokai Jan 19, 2012

Contributor

I agree that an explicit unit test would better communicate the intended behavior. I did originally mean it to have the .update() semantics so that you would "inherit" the existing mapping (if available) and then be able to (possibly partially) override it with the translator() level mapping.

The API docs state that

"""The optional mapping argument can specify or override the tstring interpolation mapping"""

and reading that again does make me think that the "override" part would be better implemented as "replaces the original mapping" instead of "updates the original mapping". I don't really have a strong opinion about this since in most cases I believe you would end up providing a complete mapping in the translator() call anyway.

I'd be happy to change the patch to just overriding tstring.mapping. Opinions?

This comment has been minimized.

Show comment Hide comment
@mgedmin

mgedmin Jan 19, 2012

I actually like the .update(). In what circumstances would you want to interpolate with an incomplete mapping? Maybe to discover errors early? E.g. original string and its mapping changed, and you'd rather learn about that so you can compute your updated mapping fully, instead of relying on the new mapping item shining through from underneath?

I don't know. API design is not my strong side.

@mgedmin

mgedmin Jan 19, 2012

I actually like the .update(). In what circumstances would you want to interpolate with an incomplete mapping? Maybe to discover errors early? E.g. original string and its mapping changed, and you'd rather learn about that so you can compute your updated mapping fully, instead of relying on the new mapping item shining through from underneath?

I don't know. API design is not my strong side.

This comment has been minimized.

Show comment Hide comment
@dokai

dokai Jan 21, 2012

Contributor

I added a test to demonstrate the case of partial overrides to the interpolation mappings.

@dokai

dokai Jan 21, 2012

Contributor

I added a test to demonstrate the case of partial overrides to the interpolation mappings.

wichert added a commit that referenced this pull request Feb 3, 2012

Merge pull request #5 from dokai/master
Fix for passing interpolation mappings in translator calls.

@wichert wichert merged commit 3afa341 into Pylons:master Feb 3, 2012

wichert added a commit that referenced this pull request Feb 3, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment