◐ Shell
reader mode source ↗
Skip to content

bpo-28180: Fix the implementation of PEP 538 on Android#4334

Merged
xdegaye merged 5 commits into
python:masterfrom
xdegaye:bpo-28180
Nov 12, 2017
Merged

bpo-28180: Fix the implementation of PEP 538 on Android#4334
xdegaye merged 5 commits into
python:masterfrom
xdegaye:bpo-28180

Conversation

@xdegaye

@xdegaye xdegaye commented Nov 8, 2017

Copy link
Copy Markdown
Contributor

@xdegaye xdegaye added skip news type-feature A feature request or enhancement labels Nov 8, 2017
@xdegaye xdegaye requested a review from ncoghlan November 8, 2017 11:08
@xdegaye xdegaye changed the title Fix the implementation of PEP 538 on Android Nov 8, 2017
@xdegaye

xdegaye commented Nov 8, 2017

Copy link
Copy Markdown
Contributor Author

Reminder:
Prefix the commit msg with bpo-28180 before merging.

@vstinner

vstinner commented Nov 8, 2017

Copy link
Copy Markdown
Member

Is the "C.UTF-8" locale available on Android?

@xdegaye

xdegaye commented Nov 8, 2017

Copy link
Copy Markdown
Contributor Author

Android has the "C.UTF-8" and "C" locales, see setlocale.

@ncoghlan ncoghlan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

If I'm reading the code correctly, I think the outcome you're aiming for can be achieved more simply with one or two unconditional setenv calls.

@bedevere-bot

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@xdegaye

xdegaye commented Nov 11, 2017

Copy link
Copy Markdown
Contributor Author

With the implementation of _Py_SetLocaleFromEnv(), the Android specific code path in the implementation of PEP 538 is now limited to _Py_SetLocaleFromEnv() itself.

When one of the locale envt variable is set to 'C', 'POSIX' or an invalid locale, then locale coercion is done with the corresponding warnings printed as for the other *nix platforms, and this can be overriden with the envt variable PYTHONCOERCECLOCALE.

@xdegaye

xdegaye commented Nov 11, 2017

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again.

@bedevere-bot

Copy link
Copy Markdown

Thanks for making the requested changes!

@ncoghlan: please review the changes made to this pull request.

@ncoghlan ncoghlan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

Nice, I like this approach to consolidating the platform dependent logic.

@ncoghlan

Copy link
Copy Markdown
Contributor

@xdegaye I'm wondering if we should actually add a short NEWS entry for this, though - that way there will be a clearer record as to when we consolidated the platform dependent handling of setting the locale from the environment, which should be useful for folks embedding CPython in larger applications.

@xdegaye xdegaye removed the skip news label Nov 12, 2017
@xdegaye xdegaye closed this Nov 12, 2017
@xdegaye xdegaye reopened this Nov 12, 2017
@xdegaye

xdegaye commented Nov 12, 2017

Copy link
Copy Markdown
Contributor Author

Closing and re-opening the PR because travis was hanged without starting.
This seems to happen when a new commit is being pushed while travis is still running on the previous commit.

@xdegaye xdegaye merged commit 1588be6 into python:master Nov 12, 2017
@xdegaye xdegaye deleted the bpo-28180 branch November 12, 2017 11:46
yan12125 pushed a commit to yan12125/python3-android that referenced this pull request Nov 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type-feature A feature request or enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants