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.

Sign up for GitHub

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

Jump to bottom

Removed hardcoded PATHs in test #27

Merged
merged 3 commits into from
Oct 13, 2022
Merged

Removed hardcoded PATHs in test #27

merged 3 commits into from
Oct 13, 2022

Conversation

thiagokokada
Copy link
Contributor

@thiagokokada thiagokokada commented Oct 12, 2022 •
edited

Some distros like NixOS does not have binaries on /bin. Instead of hardcoding them, try to get them from PATH using shutil.which function.

@thiagokokada
Do not hardcode PATHs in test
6673ee6 
Some distros like NixOS does not have binaries on `/bin`. Instead of
hardcoding them, try to get them from PATH using shutil.which function.
@thiagokokada
Copy link
Contributor Author

Should have done this before opening PR #26. Now I can run the tests locally with success.

@thiagokokada
Copy link
Contributor Author

thiagokokada commented Oct 12, 2022 •
edited

Sorry for all the different PRs.

With this PR and #28 I finally can run tox locally successfully.

@mristin mristin changed the title Do not hardcode PATHs in test Remove hardcoded PATHs in test Oct 13, 2022
@mristin mristin changed the title Remove hardcoded PATHs in test Removed hardcoded PATHs in test Oct 13, 2022
mristin
mristin requested changes Oct 13, 2022
View reviewed changes
Copy link
Collaborator

@mristin mristin left a comment •
edited

Choose a reason for hiding this comment

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

Thanks! LGTM! Please see the suggested note to be added as a comment. (I'm on my phone, so the column width might need to be fixed if I got it wrong.)

tests/test_ldd.py Show resolved Hide resolved
mristin
mristin requested changes Oct 13, 2022
View reviewed changes
Copy link
Collaborator

@mristin mristin left a comment •
edited

Choose a reason for hiding this comment

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

LGTM! I made a minor suggestion to explain in the code why shutil.which is used for the readers who do not instinctively run git blame. (double-post due to slow internet)

@thiagokokada @mristin
Add comment about shutil.which usage
6c660e4 
Co-authored-by: Marko Ristin <marko@ristin.ch>
@mristin mristin merged commit 4022994 into Parquery:master Oct 13, 2022
@mristin
Copy link
Collaborator

mristin commented Oct 13, 2022

@thiagokokada thanks again! I'll bump a patch version as soon as I come to a desktop computer. You plan no further immediate PRs, right?

@thiagokokada thiagokokada deleted the do-not-hardcode-paths branch October 13, 2022 07:56
@thiagokokada
Copy link
Contributor Author

@thiagokokada thanks again! I'll bump a patch version as soon as I come to a desktop computer. You plan no further immediate PRs, right?

I don't plan any other PRs for now.

Thanks for the reviews and the work in the new release 😄 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mristin mristin mristin requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@thiagokokada @mristin