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

fix: nox.session.run-ing commands with pathlib.Path arguments #649

Merged
merged 2 commits into from
Oct 7, 2022

Conversation

Julian
Copy link
Contributor

@Julian Julian commented Sep 4, 2022

Everything previously really worked fine in theory, but the debug string
which is logged (and calls shlex.quote) was blowing up trying
to quote a pathlib.Path object. I'm somewhat surprised upstream
shlex.quote doesn't support pathlib.Path objects (yet?) but for
now this just falls back to a cruder representation.

(Obviously there are other ways to handle this, like calling str() on all the args, let me know if you prefer something else).

This also apparently doesn't work on Windows, but that's because subprocess.Popen is apparently broken with WindowsPaths. The upstream bug is python/cpython#85815.

@Julian Julian force-pushed the pathlib-run branch 4 times, most recently from 3275143 to 7e0c7d7 Compare September 4, 2022 09:28
Previously this should have worked in theory, but the string
which is logged for debugging calls shlex.quote, which was
blowing up trying to quote the pathlib.Path.

I'm slightly surprised shlex.quote doesn't support pathlib.Path
objects (yet?) but for now here this just falls back to a cruder
representation when quoting fails.
@henryiii
Copy link
Collaborator

henryiii commented Sep 7, 2022

I'd think os.fspath(x) if isinstance(x, os.PathLike) for x in args would be the right way to handle it (without introducing bugs if something non-pathlike gets passed in that has a str representation - which everything in Python does).

@Julian
Copy link
Contributor Author

Julian commented Sep 8, 2022

Happy to do that instead obviously -- do you mean just what I just pushed (I don't think one needs to do the isinstance check, unless you meant to filter out non-fspath args silently, which seems unlikely)?

@henryiii
Copy link
Collaborator

henryiii commented Sep 8, 2022

Actually, I guess it doesn't need a check, since os.fspath passes through str / bytes and only works on os.PathLike otherwise. It just seems a little odd to pass a non-path into os.fspath, and there will be non-paths coming in. It also might be a bit more extendable - if any other object protocols were ever added, you'd then need the check.

@henryiii
Copy link
Collaborator

henryiii commented Sep 8, 2022

FYI, I recently wrote a class that wrapped a commandline tool, and realized I could define __fspath__() instead of making up an arbitrary attribute holding the path to the wrapped exe. Things like this would cause it to just work as if it was the wrapped exe (this is exactly what I did with my own run function). Even without it, os.fspath(thing) is easier to remember than thing.some_name_I_made_up.

But the important thing is the str is different than __fspath__(), and the fspath one is the correct one to use.

@Julian
Copy link
Contributor Author

Julian commented Sep 8, 2022

To os.fspath you mean? That's why the way I did it originally seemed slightly better to me, since really what should work is "anything that subprocess is happy with should work here too", and it's nice if that automatically stays true without needing to make sure it does (considering there's some side use here of printing a debug string which indeed has different requirements). Especially so since I suspected from the comment that there's intention to remove _shlex_join if/when you stop supporting <3.8, so having it do something more than what the "real" shlex.join does is slightly awkward.

But obviously I'm happy to do whatever you'd like here, just lemme know what you suggest.

@Julian
Copy link
Contributor Author

Julian commented Sep 8, 2022

Right I've made CLIs that worked precisely as you describe (where they allow lots of fun objects to become filepaths). Certainly supporting more objects would indeed be nice.

@Julian
Copy link
Contributor Author

Julian commented Oct 6, 2022

Anything else you'd like to see changed from this one?

@henryiii
Copy link
Collaborator

henryiii commented Oct 7, 2022

I think this is great, and would avoid me having to write things like:

session.run("check-wheel-contents", str(*Path("dist").glob("*.whl")), "--ignore=W002")

That obviously only works if one wheel is found; if I could get rid of the str, it would handle any number of wheels (or I could use a comprehension, but that's already more complex then I want for something like this).

Copy link
Collaborator

@FollowTheProcess FollowTheProcess left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was initially concerned about using os.fspath on any argument, whether or not it is "PathLike". But it turns out that if os.fspath gets passed str or bytes they are returned unchanged so no worries there! Looks good to me @Julian thanks!

Link to os.fspath: https://docs.python.org/3/library/os.html#os.fspath

@FollowTheProcess FollowTheProcess merged commit 07fb499 into wntrblm:main Oct 7, 2022
@Julian Julian deleted the pathlib-run branch October 7, 2022 16:58
@Julian
Copy link
Contributor Author

Julian commented Oct 7, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants