[Python-Dev] proposed os.fspath() change

[Brett Cannon]

On Wed, 15 Jun 2016 at 11:39 Guido van Rossum <guido at python.org> wrote:

> OK, so let's add a check on the return of __fspath__() and keep the check
> on path-like or string/bytes.
>

I'll update the PEP.

Ethan, do you want to leave a note on the os.fspath() issue to update the
code and go through where we've used os.fspath() to see where we can cut
out redundant type checks?


> --Guido (mobile)
> On Jun 15, 2016 11:29 AM, "Nick Coghlan" <ncoghlan at gmail.com> wrote:
>
>> On 15 June 2016 at 10:59, Brett Cannon <brett at python.org> wrote:
>> >
>> >
>> > On Wed, 15 Jun 2016 at 09:48 Guido van Rossum <guido at python.org> wrote:
>> >>
>> >> These are really two separate proposals.
>> >>
>> >> I'm okay with checking the return value of calling obj.__fspath__;
>> that's
>> >> an error in the object anyways, and it doesn't matter much whether we
>> do
>> >> this or not (though when approving the PEP I considered this and
>> decided not
>> >> to insert a check for this). But it doesn't affect your example, does
>> it? I
>> >> guess it's easier to raise now and change the API in the future to
>> avoid
>> >> raising in this case (if we find that raising is undesirable) than the
>> other
>> >> way around, so I'm +0 on this.
>> >
>> > +0 from me as well. I know in some code in the stdlib that has been
>> ported
>> > which prior to adding support was explicitly checking for str/bytes this
>> > will eliminate its own checking (obviously not a motivating factor as
>> it's
>> > pretty minor).
>>
>> I'd like a strong assertion that the return value of os.fspath() is a
>> plausible filesystem path representation (so either bytes or str), and
>> *not* some other kind of object that can also be used for accessing
>> the filesystem (like a file descriptor or an IO stream)
>>
>> >> The other proposal (passing anything that's not understood right
>> through)
>> >> is more interesting and your use case is somewhat compelling. Catching
>> the
>> >> exception coming out of os.fspath() would certainly be much messier.
>> The
>> >> question remaining is whether, when this behavior is not desired (e.g.
>> when
>> >> the caller of os.fspath() just wants a string that it can pass to
>> open()),
>> >> the condition of passing that's neither a string not supports
>> __fspath__
>> >> still produces an understandable error. I'm not sure that that's the
>> case.
>> >> E.g. open() accepts file descriptors in addition to paths, but I'm not
>> sure
>> >> that accepting an integer is a good idea in most cases -- it either
>> gives a
>> >> mystery "Bad file descriptor" error or starts reading/writing some
>> random
>> >> system file, which it then closes once the stream is closed.
>> >
>> > The FD issue of magically passing through an int was also a concern when
>> > Ethan brought this up in an issue on the tracker. My argument is that
>> FDs
>> > are not file paths and so shouldn't magically pass through if we're
>> going to
>> > type-check anything or claim os.fspath() only works with paths (FDs are
>> > already open file objects). So in my view  either we go ahead and
>> type-check
>> > the return value of __fspath__() and thus restrict everything coming
>> out of
>> > os.fspath() to Union[str, bytes] or we don't type check anything and be
>> > consistent that os.fspath() simply does is call __fspath__() if present.
>> >
>> > And just  because I'm thinking about it, I would special-case the FDs,
>> not
>> > os.PathLike (clearer why you care and faster as it skips the override of
>> > __subclasshook__):
>> >
>> > # Can be a single-line ternary operator if preferred.
>> > if not isinstance(filename, int):
>> >     filename = os.fspath(filename)
>>
>> Note that the LZMA case Ethan cites is one where the code accepts
>> either an already opened file-like object *or* a path-like object, and
>> does different things based on which it receives.
>>
>> In that scenario, rather than introducing an unconditional "filename =
>> os.fspath(filename)" before the current logic, it makes more sense to
>> me to change the current logic to use the new protocol check rather
>> than a strict typecheck on str/bytes:
>>
>>     if isinstance(filename, os.PathLike): # Changed line
>>         filename = os.fspath(filename)    # New line
>>         if "b" not in mode:
>>             mode += "b"
>>         self._fp = builtins.open(filename, mode)
>>         self._closefp = True
>>         self._mode = mode_code
>>     elif hasattr(filename, "read") or hasattr(filename, "write"):
>>         self._fp = filename
>>         self._mode = mode_code
>>     else:
>>         raise TypeError(
>>              "filename must be a path-like or file-like object"
>>               )
>>
>> I *don't* think it makes sense to weaken the guarantees on os.fspath
>> to let it propagate non-path-like objects.
>>
>> Cheers,
>> Nick.
>>
>> --
>> Nick Coghlan   |   ncoghlan at gmail.com   |   Brisbane, Australia
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/python-dev/attachments/20160615/0b1154e4/attachment.html>