Skip to content

Comments

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

Merged
ambv merged 3 commits intopython:3.9from
ambv:revert_importlib_resources
Jun 9, 2020
Merged

[3.9] bpo-40924: Revert "bpo-39791 native hooks for importlib.resources.files (GH-20576)"#20760
ambv merged 3 commits intopython:3.9from
ambv:revert_importlib_resources

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

bedevere-bot commented Jun 9, 2020

@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

@vstinner vstinner vstinner approved these changes

@ned-deily ned-deily ned-deily 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