You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
I don't think this is necessarily an "obvious" change to make. We don't currently validate anything about the path, so it works on relative and absolute paths alike. This would introduce validation at the start, making new assumptions about the type of path we've been given.
Meanwhile, there's nothing actually bad about splitting the share name like this? No file operation is going to work on it anyway, so you're about to get an error even if you don't at this step, and if you know you've got a UNC share or a directory path, why are you asking for its suffix?
I feel like this is too risky to backport, and I'm not convinced of the value going forward given that we currently don't require a full or relative path (and probably shouldn't).
I think we should close. Fixing this would suggest we should also detect directories with . and avoid splitting those, and that to me is clearly crossing the line. I'd rather keep this function simple and let it blindly split at the last dot after the last separator.
I'm sure I can come up with a justification, but really, both operations are somewhat nonsense (or more politely, a boundary condition), so the result can be arbitrary. Arbitrarily, I'd like the splitext implementation that is simpler, faster, and more reliable.
(I guess you could reasonably argue that split(path_with_no_basename) is a valid base case for a recursive algorithm, but I think you'd struggle to say the same for splitext(path_with_no_filename). So there's a justification. But it's really just the simpler implementation that I want.)
Activity
zooba commentedon Feb 26, 2024
I don't think this is necessarily an "obvious" change to make. We don't currently validate anything about the path, so it works on relative and absolute paths alike. This would introduce validation at the start, making new assumptions about the type of path we've been given.
Meanwhile, there's nothing actually bad about splitting the share name like this? No file operation is going to work on it anyway, so you're about to get an error even if you don't at this step, and if you know you've got a UNC share or a directory path, why are you asking for its suffix?
I feel like this is too risky to backport, and I'm not convinced of the value going forward given that we currently don't require a full or relative path (and probably shouldn't).
pythongh-115892: Fix ntpath.splitext() with UNC paths
nineteendo commentedon Apr 26, 2024
It's it worth fixing this with splitroot? Or do we close this?
zooba commentedon Apr 29, 2024
I think we should close. Fixing this would suggest we should also detect directories with
.
and avoid splitting those, and that to me is clearly crossing the line. I'd rather keep this function simple and let it blindly split at the last dot after the last separator.nineteendo commentedon Apr 29, 2024
I don't think that's what @barneygale had in mind. He doesn't like the inconsistency with
ntpath.split()
:zooba commentedon Apr 29, 2024
I'm sure I can come up with a justification, but really, both operations are somewhat nonsense (or more politely, a boundary condition), so the result can be arbitrary. Arbitrarily, I'd like the
splitext
implementation that is simpler, faster, and more reliable.(I guess you could reasonably argue that
split(path_with_no_basename)
is a valid base case for a recursive algorithm, but I think you'd struggle to say the same forsplitext(path_with_no_filename)
. So there's a justification. But it's really just the simpler implementation that I want.)