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

[Nick Coghlan]

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