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

Add missed stream argument #111775

Merged
merged 2 commits into from Feb 20, 2024
Merged

Add missed stream argument #111775

merged 2 commits into from Feb 20, 2024

Conversation

shadchin
Copy link
Contributor

@shadchin shadchin commented Nov 6, 2023

No description provided.

@gaogaotiantian
Copy link
Member

This issue has been there since day 1 and no one finds it in 2 years. It's not documented and it's not used internally. Is this just dead code @jaraco ?

@jaraco
Copy link
Member

jaraco commented Nov 7, 2023

Thanks @gaogaotiantian for tracking down the origin of the issue. Although that's day 1 for this class, the code is derived from resources.py in that commit.

In the upstream implementation (which probably has better fidelity of the history of this change), that file is skipped for coverage checks. That module was provided as an illustration of the lowest level interfaces required to supply TraversableResources. It's entirely conceivable that no one has made use of them. This code was made in response to python/importlib_resources#90.

If we accept this change (and we probably should), we'll want to make sure it gets ported upstream.

@jaraco
Copy link
Member

jaraco commented Nov 7, 2023

I've cherry-picked this change into python/importlib_resources and released it as 6.1.1. This change will make it into CPython in due time. If you want to author a news fragment, I'll merge this change.

@jaraco jaraco added skip issue needs backport to 3.11 only security fixes needs backport to 3.12 bug and security fixes labels Nov 7, 2023
jaraco
jaraco previously requested changes Nov 7, 2023
View reviewed changes
Copy link
Member

@jaraco jaraco left a comment

Choose a reason for hiding this comment

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

Please add a news fragment, but otherwise looks good.

@bedevere-app
Copy link

bedevere-app bot commented Nov 7, 2023

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.

@gaogaotiantian
Copy link
Member

My question is, if no one has used this piece of code for 2 years and it's not documented at all, do we still need it in CPython repo?

@jaraco
Copy link
Member

jaraco commented Nov 7, 2023

My question is, if no one has used this piece of code for 2 years and it's not documented at all, do we still need it in CPython repo?

Probably not, but to remove it will require a deprecation and we'll want to present that deprecation first in importlib_resources. Feel free to propose that there. In the meantime, it makes sense to merge the bugfix.

@gaogaotiantian
Copy link
Member

Oh, I thought removing code that we never documented does not require anything. But yeah, the fix itself definitely makes sense.

@shadchin
Copy link
Contributor Author

shadchin commented Nov 7, 2023

I have made the requested changes; please review again

@jaraco jaraco dismissed their stale review November 7, 2023 13:47

Looks good

@jaraco jaraco requested review from jaraco and removed request for warsaw and FFY00 November 7, 2023 13:49
@shadchin
Copy link
Contributor Author

Is something required of me here?

bmwiedemann pushed a commit to bmwiedemann/openSUSE that referenced this pull request Dec 14, 2023
@bmwiedemann
Update python-importlib-resources to version 6.1.1 / rev 11 via SR 11…
70123d5 
…29224

https://build.opensuse.org/request/show/1129224
by user dirkmueller + anag+factory
- update to 6.1.1:
  * Added missed stream argument in simple.ResourceHandle. Ref
    python/cpython#111775.
  * MultiplexedPath now expects Traversable paths. String
    arguments to MultiplexedPath are now deprecated.
  * Enabled support for resources in namespace packages in zip
    files. (#287)

- Update to v5.10.1
  * #259: files no longer requires the anchor to be specified and can infer the anchor from the caller's scope (defaults to the caller's module).
  * bpo-41490: contents is now also more aggressive about consuming
  * #110 and bpo-41490: path method is more aggressive about
    releasing handles to zipfile objects early, enabling use-cases
    like certifi to leave the context open but delete the
  * Package no longer exposes importlib_resources.__version__.
    Users that w
@shadchin
Copy link
Contributor Author

What needs to be done to merge this PR?

@jaraco
Copy link
Member

jaraco commented Feb 20, 2024

Thanks for your patience. I had a rough winter, so missed the notifications. Merging now.

@jaraco jaraco enabled auto-merge (squash) February 20, 2024 14:07
@jaraco jaraco merged commit 1ff6c14 into python:main Feb 20, 2024
32 checks passed
@miss-islington-app
Copy link

Thanks @shadchin for the PR, and @jaraco for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 20, 2024
@shadchin @miss-islington
Add missed stream argument (pythonGH-111775)
11ce8c6 
* Add missed `stream` argument

* Add news
(cherry picked from commit 1ff6c14)

Co-authored-by: Alexander Shadchin <shadchin@yandex-team.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 20, 2024
@shadchin @miss-islington
Add missed stream argument (pythonGH-111775)
bbe98e8 
* Add missed `stream` argument

* Add news
(cherry picked from commit 1ff6c14)

Co-authored-by: Alexander Shadchin <shadchin@yandex-team.com>
@bedevere-app
Copy link

bedevere-app bot commented Feb 20, 2024

GH-115716 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Feb 20, 2024
@bedevere-app
Copy link

bedevere-app bot commented Feb 20, 2024

GH-115717 is a backport of this pull request to the 3.11 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.11 only security fixes label Feb 20, 2024
jaraco pushed a commit that referenced this pull request Feb 20, 2024
@miss-islington @shadchin
[3.12] Add missed stream argument (GH-111775) (#115716)
20907ca 
Add missed `stream` argument (GH-111775)

* Add missed `stream` argument

* Add news
(cherry picked from commit 1ff6c14)

Co-authored-by: Alexander Shadchin <shadchin@yandex-team.com>
jaraco pushed a commit that referenced this pull request Feb 20, 2024
@miss-islington @shadchin
[3.11] Add missed stream argument (GH-111775) (#115717)
aff083e 
Add missed `stream` argument (GH-111775)

* Add missed `stream` argument

* Add news
(cherry picked from commit 1ff6c14)

Co-authored-by: Alexander Shadchin <shadchin@yandex-team.com>
@shadchin shadchin deleted the patch-2 branch February 20, 2024 17:50
woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
@shadchin @woodruffw
Add missed stream argument (python#111775)
468e14a 
* Add missed `stream` argument

* Add news
@srittau srittau mentioned this pull request Mar 30, 2024
Merged
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
@shadchin @diegorusso
Add missed stream argument (python#111775)
7df0390 
* Add missed `stream` argument

* Add news
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaraco jaraco jaraco approved these changes

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@shadchin @gaogaotiantian @jaraco