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.

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

gh-101144: Allow open and read_text encoding to be positional. #101145

Merged

Conversation

gpshead
Copy link
Member

@gpshead gpshead commented Jan 19, 2023

This was the behavior in 3.9 and earlier. The fix for #87817 introduced an API regression in 3.10.0b1.

As was the behavior in 3.9 and earlier.  The fix for
python#87817 introduced an API
regression in 3.10.0b1.
@gpshead gpshead self-assigned this Jan 19, 2023
@gpshead gpshead added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes type-bug An unexpected behavior, bug, or error stdlib Python modules in the Lib dir labels Jan 19, 2023
Lib/zipfile/_path.py Outdated Show resolved Hide resolved
This preserves the intent of
python#87817 while restoring the
ability to pass `encoding` as a positional argument.
@gpshead gpshead marked this pull request as ready for review January 19, 2023 02:30
@gpshead gpshead requested a review from jaraco as a code owner January 19, 2023 02:30
@gpshead
Copy link
Member Author

gpshead commented Jan 19, 2023

(internally at work the bug this fixes comes from b/266007820)

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.

This change will need to be ported to zipp as well.

If only the affected user had been using the backport for forward-compatibility, they would have discovered this issue much earlier. On further consideration, I guess it's not a matter of forward compatibility (as zipp will often be ahead of some slightly stale Python version). The issue really lies in that the code is only tested on one major version.

This change is mostly fine, but I have some concerns I'd like to see investigated/addressed before proceeding.

Lib/zipfile/_path.py Outdated Show resolved Hide resolved
Lib/zipfile/_path.py Outdated Show resolved Hide resolved
Lib/zipfile/_path.py Show resolved Hide resolved
@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

Lib/zipfile/_path.py Outdated Show resolved Hide resolved
Lib/zipfile/_path.py Outdated Show resolved Hide resolved
Lib/zipfile/_path.py Outdated Show resolved Hide resolved
@gpshead
Copy link
Member Author

gpshead commented Jan 20, 2023

I have made the requested changes; please review again.

@bedevere-bot
Copy link

Thanks for making the requested changes!

@methane, @jaraco: please review the changes made to this pull request.

Doc/library/zipfile.rst Outdated Show resolved Hide resolved
@gpshead gpshead merged commit 5927013 into python:main Jan 20, 2023
@miss-islington
Copy link
Contributor

Thanks @gpshead for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

@gpshead gpshead deleted the regression/zipfile.Path.read_text/gh-101144 branch January 20, 2023 07:04
@miss-islington
Copy link
Contributor

Sorry, @gpshead, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 5927013e47a8c63b70e104152351f3447baa819c 3.11

@miss-islington
Copy link
Contributor

Sorry @gpshead, I had trouble checking out the 3.10 backport branch.
Please retry by removing and re-adding the "needs backport to 3.10" label.
Alternatively, you can backport using cherry_picker on the command line.
cherry_picker 5927013e47a8c63b70e104152351f3447baa819c 3.10

gpshead added a commit to gpshead/cpython that referenced this pull request Jan 20, 2023
…ional. (pythonGH-101145)

The zipfile.Path open() and read_text() encoding parameter can be supplied as a positional argument without causing a TypeError again. 3.10.0b1 included a regression that made it keyword only.

Documentation update included as users writing code to be compatible with a wide range of versions will need to consider this for some time..
(cherry picked from commit 5927013)

Co-authored-by: Gregory P. Smith <[email protected]>
@gpshead gpshead removed the needs backport to 3.10 only security fixes label Jan 20, 2023
@gpshead
Copy link
Member Author

gpshead commented Jan 20, 2023

#101179 applies it to 3.11 and will be proposed as a 3.10 backport, though we may just update the 3.10 docs and not fix it in that branch.

@gpshead gpshead removed the needs backport to 3.11 only security fixes label Jan 20, 2023
jaraco pushed a commit to jaraco/zipp that referenced this pull request Jan 27, 2023
…onal. (python/cpython#101145)

The zipfile.Path open() and read_text() encoding parameter can be supplied as a positional argument without causing a TypeError again. 3.10.0b1 included a regression that made it keyword only.

Documentation update included as users writing code to be compatible with a wide range of versions will need to consider this for some time.
jaraco pushed a commit to jaraco/zipp that referenced this pull request Jan 27, 2023
…onal. (python/cpython#101145)

The zipfile.Path open() and read_text() encoding parameter can be supplied as a positional argument without causing a TypeError again. 3.10.0b1 included a regression that made it keyword only.

Documentation update included as users writing code to be compatible with a wide range of versions will need to consider this for some time.
clrpackages pushed a commit to clearlinux-pkgs/pypi-zipp that referenced this pull request Jan 30, 2023
…n 3.12.0

Gregory P. Smith (1):
      python/cpython#101144: Allow open and read_text encoding to be positional. (python/cpython#101145)

Jason R. Coombs (13):
      Honor ResourceWarnings. Fixes jaraco/skeleton#73.
      tox 4 requires a boolean value, so use '1' to FORCE_COLOR. Fixes jaraco/skeleton#74.
      Remove unnecessary shebang and encoding header in docs conf.
      Prevent Python 3.12 from blocking checks.
      Build docs in CI, including sphinx-lint.
      Put tidelift docs dependency in its own section to limit merge conflicts.
      Update badge for 2023
      Update changelog. Ref python/cpython#101144.
      Invoke test_encoding_warnings in-process, but skip when warn_default_encoding is not set. Re-use alpharep fixture for the file.
      Provide 'sys.flags.warn_default_encoding' for the tests to skip prior to 3.10.
      Due to mypy, it's not possible to patch the value, so just be lenient in access.
      Prefer simple asserts
      Replace trailing comment with a comment on a separate line.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants