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

bpo-29755: Fixed the lgettext() family of functions in the gettext module. #2266

Merged
merged 4 commits into from Jun 20, 2017

Conversation

Projects
None yet
5 participants
@serhiy-storchaka
Member

serhiy-storchaka commented Jun 18, 2017

They now always return bytes.

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Jun 18, 2017

@serhiy-storchaka, thanks for your PR! By analyzing the history of the files in this pull request, we identified @birkenfeld, @warsaw and @loewis to be potential reviewers.

mention-bot commented Jun 18, 2017

@serhiy-storchaka, thanks for your PR! By analyzing the history of the files in this pull request, we identified @birkenfeld, @warsaw and @loewis to be potential reviewers.

@warsaw

This comment has been minimized.

Show comment
Hide comment
@warsaw

warsaw Jun 18, 2017

Member

LGTM, and thanks for submitting this! Some thoughts:

  • I think the docs should explicitly discourage the l*() methods. I agree with your comment in the bpo-29755 that there's probably almost no utility for them in Python 3. I'm not sure we should go as far as deprecate them, but we should strong steer people clear of using them in favor of gettext() and Unicodes all the way down.
  • These methods should document that exceptions are now possible if the strings can't be encoded to bytes in the given codeset.
Member

warsaw commented Jun 18, 2017

LGTM, and thanks for submitting this! Some thoughts:

  • I think the docs should explicitly discourage the l*() methods. I agree with your comment in the bpo-29755 that there's probably almost no utility for them in Python 3. I'm not sure we should go as far as deprecate them, but we should strong steer people clear of using them in favor of gettext() and Unicodes all the way down.
  • These methods should document that exceptions are now possible if the strings can't be encoded to bytes in the given codeset.
@warsaw

warsaw approved these changes Jun 18, 2017

A couple of additional suggestions were given, but I know you'll DTRT and what you have is already good, so I'll approve it in advance.

@serhiy-storchaka

This comment has been minimized.

Show comment
Hide comment
@serhiy-storchaka

serhiy-storchaka Jun 18, 2017

Member

I'm interesting in your suggestions. In any case I'm going to rewrite the docs (group the l*() functions for avoiding duplications).

Member

serhiy-storchaka commented Jun 18, 2017

I'm interesting in your suggestions. In any case I'm going to rewrite the docs (group the l*() functions for avoiding duplications).

@warsaw

This comment has been minimized.

Show comment
Hide comment
@warsaw

warsaw Jun 19, 2017

Member

Grouping the l* methods sounds like a great idea. I'd probably just put a reST admonition at the top of that section. Draft text:

.. warning:
   These `l*()` methods should be avoided in Python 3, because they return encoded bytes.
   It's much better to use alternatives which return Unicode strings instead, since most Python
   applications will want to manipulate human readable text as strings instead of bytes.  Further,
   it's possible that you may get unexpected Unicode-related exceptions if there are encoding
   problems with the translated strings.  It is possible that the `l*()` methods will be deprecated
   in future Python versions due to their inherent problems and limitations.
Member

warsaw commented Jun 19, 2017

Grouping the l* methods sounds like a great idea. I'd probably just put a reST admonition at the top of that section. Draft text:

.. warning:
   These `l*()` methods should be avoided in Python 3, because they return encoded bytes.
   It's much better to use alternatives which return Unicode strings instead, since most Python
   applications will want to manipulate human readable text as strings instead of bytes.  Further,
   it's possible that you may get unexpected Unicode-related exceptions if there are encoding
   problems with the translated strings.  It is possible that the `l*()` methods will be deprecated
   in future Python versions due to their inherent problems and limitations.
@serhiy-storchaka

This comment has been minimized.

Show comment
Hide comment
@serhiy-storchaka

serhiy-storchaka Jun 20, 2017

Member

Thank you for your review @warsaw. Please look at the updated documentation since I'm not sure about my English.

Member

serhiy-storchaka commented Jun 20, 2017

Thank you for your review @warsaw. Please look at the updated documentation since I'm not sure about my English.

@warsaw

This comment has been minimized.

Show comment
Hide comment
@warsaw

warsaw Jun 20, 2017

Member

Looks great, thanks!

Member

warsaw commented Jun 20, 2017

Looks great, thanks!

@serhiy-storchaka serhiy-storchaka merged commit 26cb465 into python:master Jun 20, 2017

2 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
bedevere/issue-number Issue number 29755 found.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@serhiy-storchaka serhiy-storchaka deleted the serhiy-storchaka:lgettext branch Jun 20, 2017

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Jun 20, 2017

[3.6] bpo-29755: Fixed the lgettext() family of functions in the gett…
…ext module. (pythonGH-2266)

They now always return bytes.

Updated the gettext documentation..
(cherry picked from commit 26cb465)
@bedevere-bot

This comment has been minimized.

Show comment
Hide comment
@bedevere-bot

bedevere-bot Jun 20, 2017

GH-2297 is a backport of this pull request to the 3.6 branch.

bedevere-bot commented Jun 20, 2017

GH-2297 is a backport of this pull request to the 3.6 branch.

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Jun 20, 2017

[3.5] bpo-29755: Fixed the lgettext() family of functions in the gett…
…ext module. (pythonGH-2266)

They now always return bytes.

Updated the gettext documentation.
(cherry picked from commit 26cb465)
@bedevere-bot

This comment has been minimized.

Show comment
Hide comment
@bedevere-bot

bedevere-bot Jun 20, 2017

GH-2298 is a backport of this pull request to the 3.5 branch.

bedevere-bot commented Jun 20, 2017

GH-2298 is a backport of this pull request to the 3.5 branch.

serhiy-storchaka added a commit that referenced this pull request Jun 20, 2017

[3.6] bpo-29755: Fixed the lgettext() family of functions in the gett…
…ext module. (GH-2266) (#2297)

They now always return bytes.

Updated the gettext documentation..
(cherry picked from commit 26cb465)

serhiy-storchaka added a commit that referenced this pull request Jun 20, 2017

[3.5] bpo-29755: Fixed the lgettext() family of functions in the gett…
…ext module. (GH-2266) (#2298)

They now always return bytes.

Updated the gettext documentation.
(cherry picked from commit 26cb465)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment