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

Add test for python executable search logic. #756

Closed
wants to merge 0 commits into from
Closed

Add test for python executable search logic. #756

wants to merge 0 commits into from

Conversation

bnoordhuis
Copy link
Member

Break out the search logic into a separate function and add a regression
test.

References: #668

R=@rvagg? /cc @refack

}
} else {
log.verbose('`which` succeeded', python, execPath)
checkPythonVersion()
Copy link
Contributor

Choose a reason for hiding this comment

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

you're missing the critical fix of using which's result

       python = execPath

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, yes, that's intentional. Mixing a general refactor and bug fixes in a single commit is bad style.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 got it.

@refack refack mentioned this pull request Oct 4, 2015
@bnoordhuis
Copy link
Member Author

Maybe R=@TooTallNate?

if (err) {
return callback(err)
}
log.verbose('check python version', '`%s -c "import platform; print(platform.python_version());"` returned: %j', python, stdout)
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we don't have the Max length linter rule.

@bnoordhuis
Copy link
Member Author

@thefourtheye What was the verdict on this PR?

@rvagg
Copy link
Member

rvagg commented Nov 18, 2015

I don't have time right now to review this but fwiw I extracted this logic into a separate package a while back: https://github.com/rvagg/check-python and figured one day I'd come back here and PR a full replacement. The logging is the tricky bit, however.

if (win) {
guessPython()
} else {
failNoPython()
Copy link
Contributor

Choose a reason for hiding this comment

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

After the refactoring, failNoPython directly calls the configure's callback and brings the execution to end. But ideally it should have called findPython's callback and that should have propagated the error back to the caller of configure. This will still work but looks like it can be improved/done better to me. Please point out if I am missing something obvious.

Copy link
Member Author

Choose a reason for hiding this comment

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

But it does, doesn't it? It calls the callback from findPython()'s arguments.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bnoordhuis I am sorry. You are correct. As the change was lengthier, I was not able to view it properly I guess. LGTM.

@thefourtheye
Copy link
Contributor

LGTM with a suggestion/question.

bnoordhuis added a commit that referenced this pull request Nov 24, 2015
Break out the search logic into a separate function and add a regression
test.

References: #668
PR-URL: #756
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@bnoordhuis bnoordhuis closed this Nov 24, 2015
@bnoordhuis bnoordhuis deleted the add-find-python-test branch November 24, 2015 14:08
@bnoordhuis
Copy link
Member Author

Landed in 817ed9b with the s/equal/strictEqual/ fix.

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.

4 participants