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

fix test_find_cmd_python #3342

Merged
merged 2 commits into from May 24, 2013
Merged

fix test_find_cmd_python #3342

merged 2 commits into from May 24, 2013

Conversation

minrk
Copy link
Member

@minrk minrk commented May 20, 2013

It's a bit unclear what should be done here. 'python' is short-circuited in find_cmd to sys.executable. This commit does the same short-circuit with basename(sys.executable), which can be any of Python, python, python3, python3.exe, etc.

An alternate fix would be to change the test back to testing what the code
actually did.

It's a bit unclear what should be done here.
'python' is short-circuited in find_cmd to sys.executable.
This commit does the same short-circuit with basename(sys.executable), which can be any of Python, python, python3, python3.exe, etc.

An alternate fix would be to change the test back to testing what the code actually did.
@takluyver
Copy link
Member

I don't think there's anywhere in the codebase that actually calls find_cmd on Python (by any name), other than the tests for it. I wonder if we should just drop the special case and the test for it, and let find_cmd look up 'python' like any other name.

Returning sys.executable might also be problematic for users with frozen code - in that case, sys.executable will be the base executable used to launch the application, not a Python interpreter.

@minrk
Copy link
Member Author

minrk commented May 21, 2013

I think that makes sense - I've removed the special case and the test.

@takluyver
Copy link
Member

Do we want to also update the Windows-only test for finding pythonw? Is there some binary that should always be on the path in Windows? I don't think it's causing any problems, though, so I'm happy to leave it as is.

@takluyver
Copy link
Member

Merging this so we can get the Windows tests passing again.

takluyver added a commit that referenced this pull request May 24, 2013
@takluyver takluyver merged commit c751c59 into ipython:master May 24, 2013
@minrk minrk deleted the executable branch March 31, 2014 23:36
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
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.

None yet

2 participants