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