X Tutup
The Wayback Machine - https://web.archive.org/web/20221208222421/https://github.com/python/cpython/pull/96194
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-96192: fix os.ismount() to use a path that is str or bytes #96194

Merged
merged 3 commits into from Nov 14, 2022

Conversation

calestyo
Copy link
Contributor

@calestyo calestyo commented Aug 23, 2022

Co-authored-by: Eryk Sun <eryksun@gmail.com>
Signed-off-by: Christoph Anton Mitterer <mail@christoph.anton.mitterer.name>
@bedevere-bot
Copy link

bedevere-bot commented Aug 23, 2022

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Aug 23, 2022

All commit authors signed the Contributor License Agreement.
CLA signed

@orsenthil
Copy link
Member

orsenthil commented Aug 24, 2022

Hi @calestyo , Would you like to add a test for this?

@orsenthil orsenthil self-assigned this Aug 24, 2022
@calestyo
Copy link
Contributor Author

calestyo commented Aug 25, 2022

@orsenthil Well I could at least try, but two issues here:

  • I think it would be handy to have a test that does this for any function that is ought to accept the PathLike-interface, but that in turn doesn't seem to be possibly generically.
  • The test get a bit ugly, since one cannot create os.DirEntry objects, even if I'd just made one for os.ismount(). Right now these do:
    def test_ismount(self):
    self.assertIs(posixpath.ismount("/"), True)
    self.assertIs(posixpath.ismount(b"/"), True)

but one cannot yield / as os.DirEntry via scandir().

@orsenthil
Copy link
Member

orsenthil commented Aug 26, 2022

Without this patch, I would expect this

self.assertIs(posixpath.ismount(b"/"), True)

to fail or behavior differently than with this patch.

What might I be missing?

@calestyo
Copy link
Contributor Author

calestyo commented Aug 26, 2022

Without this patch, I would expect this

self.assertIs(posixpath.ismount(b"/"), True)

to fail or behavior differently than with this patch.

That I don't understand… as far as I understand it, it should have worked previously with both str and bytes but not when e.g. using it with a os.DirEntry.

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Could you add a test that passes a bytes path-like to ismount()?

@bedevere-bot
Copy link

bedevere-bot commented Oct 7, 2022

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be put in the comfy chair!

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Signed-off-by: Christoph Anton Mitterer <mail@christoph.anton.mitterer.name>
@calestyo
Copy link
Contributor Author

calestyo commented Nov 13, 2022

Could you add a test that passes a bytes path-like to ismount()?

@orsenthil @JelleZijlstra … I've added a test using Path. Does that seem ok for you?

@calestyo
Copy link
Contributor Author

calestyo commented Nov 13, 2022

I have made the requested changes; please review again

@bedevere-bot
Copy link

bedevere-bot commented Nov 13, 2022

Thanks for making the requested changes!

@JelleZijlstra: please review the changes made to this pull request.

@bedevere-bot bedevere-bot requested a review from JelleZijlstra Nov 13, 2022
@JelleZijlstra
Copy link
Member

JelleZijlstra commented Nov 14, 2022

The tests are failing.

@calestyo
Copy link
Contributor Author

calestyo commented Nov 14, 2022

Uagh... seems Path itsellf claims it would accept a PathLike object, e.g. pathlib.PurePath(*pathsegments) says:

Each element of pathsegments can be either a string representing a path segment, an object implementing the os.PathLike interface which returns a string, or another path object:

but it doesn't really, but only such that use strings. Isn't that yet another bug?

Anyway... I don't know how to do a proper test then. Is there any other PathLike object type in the library?

The only I'd know is os.DirEntry which one cannot directly create. I could use os.scandir(b"/")__next__() but there's the result could be a mointpoint or not.

Any ideas?

@calestyo
Copy link
Contributor Author

calestyo commented Nov 14, 2022

Oh and is a test here really worth it? The fix is trivial, should someone ever remove it, the check could also just be removed by accident as well.

As I've already said above, to which noone replied,... it would IMO only make sense, if all these functions could be tested for whether they accept the right types, at once.

@JelleZijlstra
Copy link
Member

JelleZijlstra commented Nov 14, 2022

Uagh... seems Path itsellf claims it would accept a PathLike object, e.g. pathlib.PurePath(*pathsegments) says:

Each element of pathsegments can be either a string representing a path segment, an object implementing the os.PathLike interface which returns a string, or another path object:

but it doesn't really, but only such that use strings. Isn't that yet another bug?

The problem in your test is that Path only supports str paths, not bytes paths. This is well-established behavior and not a bug.

Anyway... I don't know how to do a proper test then. Is there any other PathLike object type in the library?

You can use test.support.os_helper.FakePath.

Oh and is a test here really worth it?

Yes, every bugfix needs an associated test.

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Signed-off-by: Christoph Anton Mitterer <mail@christoph.anton.mitterer.name>
@calestyo calestyo force-pushed the fix-path-in-os.ismount branch from 26c9eb9 to a1aac89 Compare Nov 14, 2022
@calestyo
Copy link
Contributor Author

calestyo commented Nov 14, 2022

You can use test.support.os_helper.FakePath.

thx... still fails on one platform though... no idea why

@JelleZijlstra
Copy link
Member

JelleZijlstra commented Nov 14, 2022

Looks spurious, I retried it.

@JelleZijlstra JelleZijlstra merged commit 367f552 into python:main Nov 14, 2022
15 checks passed
@miss-islington
Copy link
Contributor

miss-islington commented Nov 14, 2022

Thanks @calestyo for the PR, and @JelleZijlstra for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒🤖

@bedevere-bot
Copy link

bedevere-bot commented Nov 14, 2022

GH-99455 is a backport of this pull request to the 3.11 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 14, 2022
…ythonGH-96194)

(cherry picked from commit 367f552)

Co-authored-by: Christoph Anton Mitterer <calestyo@scientia.org>
Signed-off-by: Christoph Anton Mitterer <mail@christoph.anton.mitterer.name>
Co-authored-by: Eryk Sun <eryksun@gmail.com>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@bedevere-bot
Copy link

bedevere-bot commented Nov 14, 2022

GH-99456 is a backport of this pull request to the 3.10 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 14, 2022
…ythonGH-96194)

(cherry picked from commit 367f552)

Co-authored-by: Christoph Anton Mitterer <calestyo@scientia.org>
Signed-off-by: Christoph Anton Mitterer <mail@christoph.anton.mitterer.name>
Co-authored-by: Eryk Sun <eryksun@gmail.com>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
miss-islington added a commit that referenced this pull request Nov 30, 2022
(cherry picked from commit 367f552)

Co-authored-by: Christoph Anton Mitterer <calestyo@scientia.org>
Signed-off-by: Christoph Anton Mitterer <mail@christoph.anton.mitterer.name>
Co-authored-by: Eryk Sun <eryksun@gmail.com>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants
X Tutup