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.

Sign up for GitHub

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

Jump to bottom

[3.9] bpo-40924: Revert "bpo-39791 native hooks for importlib.resources.files (GH-20576)" #20760

Merged
merged 3 commits into from
Jun 9, 2020
Merged

[3.9] bpo-40924: Revert "bpo-39791 native hooks for importlib.resources.files (GH-20576)" #20760

merged 3 commits into from
Jun 9, 2020

Conversation

ambv
Copy link
Contributor

@ambv ambv commented Jun 9, 2020 •
edited
Loading

This reverts commit 9cf1be4 due to

https://bugs.python.org/issue40924

The change being reverted broke Certifi and caused root certificates to be unavailable to urllib.

@vstinner
Copy link
Member

vstinner commented Jun 9, 2020

This PR looks larger than just a revert. It also upgrade to the beta2. Was it done on purpose?

@ambv
Copy link
Contributor Author

ambv commented Jun 9, 2020

No, that's a failed rebase. Fixing right now.

@ambv ambv force-pushed the revert_importlib_resources branch from 1797ad9 to c791333 Compare June 9, 2020 13:03
@ambv ambv added the skip news label Jun 9, 2020
@vstinner
Copy link
Member

vstinner commented Jun 9, 2020 •
edited by bedevere-bot
Loading

Please add a NEWS entry to mention the revert.

9cf1be4 added Misc/NEWS.d/next/Library/2020-06-02-02-16-02.[bpo-39791](https://bugs.python.org/issue39791).StCJlA.rst which was merged into 3.9.0b2.rst:

Built-in loaders (SourceFileLoader and ZipImporter) now supply ``TraversableResources`` implementations for ``ResourceReader``, and the fallback function has been removed.

vstinner
vstinner approved these changes Jun 9, 2020
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM if you add a NEWS entry.

@vstinner
Copy link
Member

vstinner commented Jun 9, 2020

IMHO it's better to document the revert than modifying Misc/NEWS.d/3.9.0b2.rst.

@ambv ambv removed the skip news label Jun 9, 2020
ned-deily
ned-deily approved these changes Jun 9, 2020
View reviewed changes
Copy link
Member

@ned-deily ned-deily left a comment

Choose a reason for hiding this comment

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

I did a complete macOS installer build and test install with the PR. The test suite ran without errors and I verified that certifi.where() now returns the correct result. With the addition of a news entry, LGTM.

ned-deily
ned-deily reviewed Jun 9, 2020
View reviewed changes
Copy link
Member

@ned-deily ned-deily left a comment

Choose a reason for hiding this comment

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

Perhaps the entry should specifically mention "introduced in 3.9.0b2"?

@ambv
Copy link
Contributor Author

ambv commented Jun 9, 2020

Good idea, Ned.

@ambv ambv merged commit ce5e6f0 into python:3.9 Jun 9, 2020
@bedevere-bot
Copy link

@ambv: Please replace # with GH- in the commit message next time. Thanks!

@ambv ambv deleted the revert_importlib_resources branch June 9, 2020 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ned-deily ned-deily ned-deily approved these changes

@vstinner vstinner vstinner approved these changes

@brettcannon brettcannon Awaiting requested review from brettcannon brettcannon is a code owner

@encukou encukou Awaiting requested review from encukou encukou is a code owner

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

@ncoghlan ncoghlan Awaiting requested review from ncoghlan ncoghlan is a code owner

@rhettinger rhettinger Awaiting requested review from rhettinger

@skrah skrah Awaiting requested review from skrah

@warsaw warsaw Awaiting requested review from warsaw warsaw is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ambv @vstinner @bedevere-bot @ned-deily @the-knights-who-say-ni