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

flit: improve the --python option #667

Merged
merged 2 commits into from
Jan 28, 2024
Merged

Conversation

perillo
Copy link
Contributor

@perillo perillo commented Dec 4, 2023

pip --python option can take both the path to a Python executable or the path to a virtual environment, greatly improving the user experience.

Teach flit to do the same.

@perillo
Copy link
Contributor Author

perillo commented Dec 4, 2023

Hello, this is my first PR for this project. I decided to adopt flit due to its simplicity.

The PR does not include a test, but it seems that there are no tests for the --python option.

flit/__init__.py Outdated Show resolved Hide resolved
@takluyver
Copy link
Member

Thanks for the contribution! I think this feature makes sense.

The PR does not include a test, but it seems that there are no tests for the --python option.

Not for passing --python itself, but there are tests for the find_python_executable() function: https://github.com/pypa/flit/blob/main/tests/test_find_python_executable.py . Could you add a test for this? It's fine to do it in a simple way, e.g. using /usr and skipping the test if /usr/bin/python doesn't exist.

@perillo
Copy link
Contributor Author

perillo commented Dec 11, 2023

The PR does not include a test, but it seems that there are no tests for the --python option.

Not for passing --python itself, but there are tests for the find_python_executable() function: https://github.com/pypa/flit/blob/main/tests/test_find_python_executable.py . Could you add a test for this? It's fine to do it in a simple way, e.g. using /usr and skipping the test if /usr/bin/python doesn't exist.

Ok. I also noted that codecov complained about it.

What about a test like:

def test_env(tmp_path):
    path = tmp_path / "venv"
    venv.create(path)
    executable = pathlib.Path(find_python_executable(path))
    assert executable.parent.parent.name == "venv" 

?

The test needs to import the venv and pathlib modules.

I would also like to add a test to ensure that a relative path to a venv is converted to an absolute path, but I'm not sure how to do it. A possible solution is to use chdir but it may cause problems.

@takluyver
Copy link
Member

That sounds reasonable, thanks.

I would also like to add a test to ensure that a relative path to a venv is converted to an absolute path, but I'm not sure how to do it. A possible solution is to use chdir but it may cause problems.

Use pytest's monkeypatch fixture, and call monkeypatch.chdir(). This records that the CWD changed in the test, and pytest will change it back afterwards (whether the test passes or not).

pip --python option can take both the path to a Python executable or the
path to a virtual environment, greatly improving the user experience.

Teach flit to do the same.
@perillo
Copy link
Contributor Author

perillo commented Dec 16, 2023

I avoided importing the pathlib module, since find_python_executable returns a string. However I directly imported isabs, basename and dirname in order to reduce the line size in test_env.

@takluyver
Copy link
Member

Thanks @perillo, nice first contribution. 🙂

@takluyver takluyver merged commit 8ead22d into pypa:main Jan 28, 2024
15 of 16 checks passed
@takluyver takluyver added this to the 3.10 milestone Jan 28, 2024
@perillo
Copy link
Contributor Author

perillo commented Jan 28, 2024

@takluyver Thanks to you.

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.

2 participants