pypa / flit

Simplified packaging of Python modules
https://flit.pypa.io/
BSD 3-Clause "New" or "Revised" License
2.16k stars 132 forks source link

flit: improve the --python option #667

Closed perillo closed 8 months ago

perillo commented 10 months ago

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 commented 10 months ago

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.

takluyver commented 10 months ago

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 commented 10 months ago

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 commented 9 months ago

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).

perillo commented 9 months ago

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 commented 8 months ago

Thanks @perillo, nice first contribution. :slightly_smiling_face:

perillo commented 8 months ago

@takluyver Thanks to you.