Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #23196 -- Don't translate empty string as gettext metadata #3192

Closed
wants to merge 1 commit into from

Conversation

@yrik
Copy link
Contributor

commented Sep 7, 2014

Returns empty value of corresponding type in case of empty string.

@yrik yrik force-pushed the yrik:ticket_23196 branch Sep 7, 2014

@gurunars

View changes

django/utils/translation/trans_real.py Outdated
if _default is None:
_default = translation(settings.LANGUAGE_CODE)
result = getattr(_default, translation_function)(eol_message)

This comment has been minimized.

Copy link
@gurunars

gurunars Sep 7, 2014

All this if nesting (lines [303-309]) is semantically equivalent to:

_default = _default or translation(settings.LANGUAGE_CODE)
t = getattr(_active, "value", _default)
result = getattr(t, translation_function)(eol_message)

This comment has been minimized.

Copy link
@gurunars

gurunars Sep 7, 2014

Could be shortened to just two lines as well:

t = getattr(_active, "value", _default) or translation(settings.LANGUAGE_CODE)
result = getattr(t, translation_function)(eol_message)

but it would decrease readability IMO

This comment has been minimized.

Copy link
@gurunars

gurunars Sep 7, 2014

Actually version two would be slightly faster, if the speed is more critical in this case

This comment has been minimized.

Copy link
@yrik

yrik Sep 7, 2014

Author Contributor

Actually this part of code[303-309] was not changed by me, I just made an indent here because of added if.
Anyway, It's good to improve the code. Your suggestion really decrease the length the code. However, the logic is changed a bit, now it fallbacks in both cases "" and None, before it was only None. I'll check if it affects on anything.

This comment has been minimized.

Copy link
@yrik

yrik Sep 7, 2014

Author Contributor

Yep, there can be only None, not ""

This comment has been minimized.

Copy link
@gurunars

gurunars Sep 7, 2014

Which makes my comment applicable, right?

This comment has been minimized.

Copy link
@yrik

yrik Sep 7, 2014

Author Contributor

Exactly!
Thanks for comment, I've updated the code.

@yrik yrik force-pushed the yrik:ticket_23196 branch Sep 7, 2014

Refs #23196 -- Don't translate empty string as gettext metadata
Added `if` before actual text translation.
Returns empty message of corresponding type instead of gettext metadata.

Added test to check if empty value stays empty value of corresponding type

Simplify code

more clear type checking

@yrik yrik force-pushed the yrik:ticket_23196 branch to f27fc59 Sep 7, 2014

result = getattr(t, translation_function)(eol_message)

if len(eol_message) == 0:
# Returns empty value of corresponding type if empty message given

This comment has been minimized.

Copy link
@timgraham

timgraham Sep 11, 2014

Member

Just corrected grammar a bit:

Returns an empty value of the corresponding type if an empty message is given,
instead of metadata, which is the default gettext behavior.
Empty value must stays empty value after translation
"""
with translation.override('de'):
s = ""

This comment has been minimized.

Copy link
@timgraham

timgraham Sep 11, 2014

Member

we are using from __future__ import unicode_literals in this file, so if you want a byte string here it should be b''

with translation.override('de'):
s = ""
self.assertEqual(s, ugettext(s))
s = u""

This comment has been minimized.

Copy link
@timgraham

timgraham Sep 11, 2014

Member

as above, this should simply be "" (u prefix is invalid on Python 3.2)

@timgraham

This comment has been minimized.

Copy link
Member

commented Sep 11, 2014

Please amend the commit message as follows (we don't need all the other details about the implementation):

Fixed #23196 -- Don't translate empty string as gettext metadata

Thanks nedbatchelder for the suggestion.
@@ -345,6 +345,18 @@ def test_string_concat(self):
"""
self.assertEqual('django', six.text_type(string_concat("dja", "ngo")))

def test_empty_value(self):
"""
Empty value must stays empty value after translation

This comment has been minimized.

Copy link
@timgraham

timgraham Sep 11, 2014

Member

Prefixing the docstring in tests with the ticket # is often helpful later: #23196 - Empty values must stay as empty values after translation.

@claudep

This comment has been minimized.

Copy link
Member

commented Sep 21, 2014

Thanks, pushed in 11f307a with suggestions fixed.

@claudep claudep closed this Sep 21, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.