zipfile.Path.read_text & .open methods with a positional encoding arg causes a TypeError

[gpshead]

This is a regression from 3.9 behavior seen in Python 3.10.

zipfile.Path.read_text passes *args and **kwargs to it's own open() method, but in 3.10+ is also blindly sets kwargs["encodings"] to a value. So code that was previously passing an encoding as a positional argument now sees:

  File "/<stdlib>/zipfile.py", line 2362, in read_text
    with self.open('r', *args, **kwargs) as strm:
  File "/<stdlib>/zipfile.py", line 2350, in open
    return io.TextIOWrapper(stream, *args, **kwargs)
TypeError: Failed to construct dataset imagenet2012: argument for TextIOWrapper() given by name ('encoding') and position (2)

3.10's Lib/zipfile.py (and main's Lib/zipfile/_path.py) contain:

    def read_text(self, *args, **kwargs):
        kwargs["encoding"] = io.text_encoding(kwargs.get("encoding"))  # <-- new in 3.10, source of bug
        with self.open('r', *args, **kwargs) as strm:
            return strm.read()

As this is a regression, we should avoid setting that value in kwargs when "encodings" not in kwargs to match 3.9 and earlier behavior.

TODO list:

• @jaraco will port the change to zipp.

Linked PRs

• gh-101144: Allow open and read_text encoding to be positional. #101145
• [3.11] gh-101144: Allow zipfile.Path .open & .read_text encoding to be positional #101179
• [3.10] gh-101144: Allow zipfile.Path .open & .read_text encoding to be positional (GH-101179) #101182

[gpshead]

(and note that 3.10 is probably closed to bugfixes)

[gpshead]

Workaround for users, always pass encoding to read_text as a keyword encoding= argument rather than the first positional argument.

[jaraco]

One thought that I keep having - although this incompatibility was introduced unintentionally, it seems not to have affected but one reported use-case (and probably a handful that were not reported). Perhaps it makes sense to retain the more constrained form. As I consider that, I realize that these wrappers are really trying to be true to the underlying interface, so it's probably preferable to honor that and remain consistent than to offer only a more constrained interface. In other words, the proposed fix sounds good.

[jaraco]

• Port the change to zipp.

Feel free to assign to me after the PR lands and I can handle the port. Or if you're interested in learning the process, I can share the process with you.

[gpshead]

Perhaps it makes sense to retain the more constrained form. As I consider that, I realize that these wrappers are really trying to be true to the underlying interface, so it's probably preferable to honor that

yep, I went through the same thought process. They're mirroring other APIs that take encoding and those accept it as a positional parameter, so fixing it rather than just declaring it keyword only made sense as a bugfix.

As for your zipp project @jaraco, I'll let you apply the change there. Keeping this open and reassigning to you for that. The 3.10 PR will probably automerge, if not one of us can push the button when CI is done.

[jaraco]

The backport ended up being fairly challenging, given the lack of sys.flags.warn_default_encoding and EncodingWarning on Python prior to 3.10, but also the nitpickiness of mypy and flake8. I could satisfy flake8 by monkeypatching builtins and sys.flags, but I couldn't satisfy mypy because it has the attributes of sys.flags hard-coded somewhere. Ultimately, I came up with something that's suitable and pretty true to the same spirit in jaraco/zipp#89 released as zipp 3.12 (the relationship to Python 3.12 is purely coincidental). The one downside is that the check for test_encoding_warnings is now skipped except in environments where the value is set. I'm assuming that the test suite is regularly tested with -X warn_default_encoding, so the test will catch a regression soon enough.

I plan to add PYTHONWARNDEFAULTENCODING to skeleton so that the projects I maintain get the warnings.