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

refactor: drop path.py #592

Closed
wants to merge 5 commits into from

Conversation

henryiii
Copy link
Collaborator

@henryiii henryiii commented Apr 6, 2022

Closes #583. Removes a deprecated dependency.

@henryiii henryiii force-pushed the henryiii/refactor/droppy branch 3 times, most recently from 2706bb4 to fd71295 Compare April 6, 2022 02:56
@henryiii
Copy link
Collaborator Author

henryiii commented Apr 6, 2022

I'm not likely to have time to power up a Windows machine to investigate why this is failing on Windows for a couple of days - in a workshop this week. If PATHEXT is off, then don't see why anything works, and if it's right, I don't see why this isn't finding things correctly. Maybe I've got a bug around py handling. 🤷

Also need to think about how to add a test for the "not found locally but found in the default path" branch for coverage, which is showing up on ubuntu except for 3.10. Which is odd.

@DiddiLeija
Copy link
Collaborator

Let me check the Windows thing around the next days 😉

@FollowTheProcess
Copy link
Collaborator

Thanks for tackling this @henryiii! I can answer the 3.10 coverage thing, it is flagging the same section of code as uncovered (see below)

image

But in the Noxfile we set the coverage fail threshold to 99% here:

nox/noxfile.py

Lines 71 to 76 in d947833

# 3.10 produces different coverage results for some reason
# see https://github.com/theacodes/nox/issues/478
fail_under = 100
py_version = sys.version_info
if py_version.major == 3 and py_version.minor == 10:
fail_under = 99

This is a hangup from when we started testing on 3.10.0-rc.2 and something about 3.10 made coverage incorrectly flag stuff as uncovered, looking at the coverage logs recently though I think we can remove this little flag (I'll do this later today).

Sadly that doesn't help us with the build failures on this PR, which I agree are a bit weird!

Also need to think about how to add a test for the "not found locally but found in the default path" branch for coverage, which is showing up on ubuntu except for 3.10. Which is odd.

Regarding this, I can't think of a scenario where this would the case? As in where would you have a program that you can't find locally but exists on $PATH? I'll have a think on how we could test for this. Great work so far though!

@henryiii
Copy link
Collaborator Author

henryiii commented Apr 7, 2022

I think such a scenario would be looking for something like git. It would not be available in the local paths that are being passed, but would be available in the system paths, so it would only be detected in the second run (AFAICT, if you give custom paths, the system paths are then not included).

Signed-off-by: Henry Schreiner <[email protected]>
@henryiii
Copy link
Collaborator Author

henryiii commented Apr 18, 2022

That's embarrassing. Just don't look in history and pretend I wrote it correctly the first time. :)

In my defense, I somehow thought the new-in-3.7 text=True parameter also implied capture_output, even though I also knew it was just an alias for universal_newlines=True. That would have been handy, I think...

Also I needed to boot up a Windows box for other things, though I do feel slightly ridiculous needed it to discover this.

I'd really have liked it if this was protected by type - if mypy could tell you couldn't access .stdout/stderr without capturing output in some way, for example!

@henryiii
Copy link
Collaborator Author

Hmm, so it looks like shutil.which expects executables to follow Window's PATHEXT and looks for .exe, .bat, and such. The tests make a no-extension executable, which shutil doesn't find but the py.path tooling did. I'll have to investigate a bit further to see what the best solution is - it's probably at most just needed in the commands.which function if a workaround is needed.

@henryiii
Copy link
Collaborator Author

henryiii commented Oct 7, 2022

Closing in favor of #647.

@henryiii henryiii closed this Oct 7, 2022
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.

Remove usage of py.path?
3 participants