Add test for python executable search logic.#756
Add test for python executable search logic.#756bnoordhuis wants to merge 0 commit intonodejs:masterfrom bnoordhuis:add-find-python-test
Conversation
There was a problem hiding this comment.
you're missing the critical fix of using which's result
python = execPath
There was a problem hiding this comment.
Well, yes, that's intentional. Mixing a general refactor and bug fixes in a single commit is bad style.
|
Maybe R=@TooTallNate? |
There was a problem hiding this comment.
I guess we don't have the Max length linter rule.
|
@thefourtheye What was the verdict on this PR? |
|
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
But it does, doesn't it? It calls the callback from findPython()'s arguments.
There was a problem hiding this comment.
@bnoordhuis I am sorry. You are correct. As the change was lengthier, I was not able to view it properly I guess. LGTM.
|
LGTM with a suggestion/question. |
Break out the search logic into a separate function and add a regression test. References: #668 PR-URL: #756 Reviewed-By: Sakthipriyan Vairamani <[email protected]>
|
Landed in 817ed9b with the |
4 participants
Break out the search logic into a separate function and add a regression
test.
References: #668
R=@rvagg? /cc @refack