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

Replace additional use of which(1) with shutil.which() #355

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mgorny
Copy link

@mgorny mgorny commented May 31, 2024

Replace the remaining use of external which(1) tool with shutil.which() from the standard Python library, finally removing the dependency on a third party package.

This is a followup to 1024f4f.

Replace the remaining use of external `which(1)` tool with
`shutil.which()` from the standard Python library, finally removing
the dependency on a third party package.

This is a followup to 1024f4f.
@ekalinin
Copy link
Owner

Hey! Thanks for the patch!
Could you, please, add some test?

@mgorny
Copy link
Author

mgorny commented Aug 25, 2024

What kind of test?

@ekalinin
Copy link
Owner

Unit test.

@mgorny
Copy link
Author

mgorny commented Aug 25, 2024

install_activate is already covered by tests. I fail to see what I could test differently there.

Copy link

@ssbarnea ssbarnea left a comment

Choose a reason for hiding this comment

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

That is indeed a very good fix, which is not installed by default on all platforms and native python implementation is well tested. Not an uncommon mistake to see in python projects.

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

Successfully merging this pull request may close these issues.

3 participants