ionelmc / cookiecutter-pylibrary

Enhanced cookiecutter template for Python libraries.
BSD 2-Clause "Simplified" License
1.25k stars 207 forks source link

Fix toxinidir handling #230

Closed cbeiraod closed 1 year ago

cbeiraod commented 2 years ago

Fixed references to toxinidir. When the directory contains spaces, the check-manifest command would fail because the path would be incorrectly interpreted as multiple options due to the spaces.

cbeiraod commented 2 years ago

I quoted this line just to be safe... there is no harm in having the quotes, but I didn't test without.

On Tue, Sep 27, 2022 at 3:25 PM Ionel Cristian Mărieș < @.***> wrote:

@.**** commented on this pull request.

In {{cookiecutter.repo_name}}/tox.ini https://github.com/ionelmc/cookiecutter-pylibrary/pull/230#discussion_r981318298 :

@@ -73,7 +73,7 @@ basepython = {%- endif -%} }: {env:TOXPYTHON:python3} setenv =

  • PYTHONPATH={toxinidir}/tests
  • PYTHONPATH='{toxinidir}/tests'

But this doesn't need to be quoted right?

— Reply to this email directly, view it on GitHub https://github.com/ionelmc/cookiecutter-pylibrary/pull/230#pullrequestreview-1122101698, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABDY23KNVSJB3FQZIPIS4MTWAL7V5ANCNFSM6AAAAAAQWW4XCI . You are receiving this because you authored the thread.Message ID: @.***>

ionelmc commented 2 years ago

Try running tox -- python -c 'import sys; print(sys.path)'

Lemme know what you get.

cbeiraod commented 2 years ago

That gives a lot of output... Is there something in particular you would want to see?

The specific output of the python command was: ['', "/Users/cristovao/Documents/Projects/PPS/LGAD Scripts/LIP_PPS_Run_Manager/'/Users/cristovao/Documents/Projects/PPS/LGAD Scripts/LIP_PPS_Run_Manager/tests'", '/usr/local/Cellar/python@3.9/3.9.10/Frameworks/Python.framework/Versions/3.9/lib/python39.zip', '/usr/local/Cellar/python@3.9/3.9.10/Frameworks/Python.framework/Versions/3.9/lib/python3.9', '/usr/local/Cellar/python@3.9/3.9.10/Frameworks/Python.framework/Versions/3.9/lib/python3.9/lib-dynload', '/Users/cristovao/Documents/Projects/PPS/LGAD Scripts/LIP_PPS_Run_Manager/.tox/py39/lib/python3.9/site-packages']

There does seem to be something weird in the second path...

and removing the quotes from the line you identified has the nicer output of: ['', '/Users/cristovao/Documents/Projects/PPS/LGAD Scripts/LIP_PPS_Run_Manager/tests', '/usr/local/Cellar/python@3.9/3.9.10/Frameworks/Python.framework/Versions/3.9/lib/python39.zip', '/usr/local/Cellar/python@3.9/3.9.10/Frameworks/Python.framework/Versions/3.9/lib/python3.9', '/usr/local/Cellar/python@3.9/3.9.10/Frameworks/Python.framework/Versions/3.9/lib/python3.9/lib-dynload', '/Users/cristovao/Documents/Projects/PPS/LGAD Scripts/LIP_PPS_Run_Manager/.tox/py39/lib/python3.9/site-packages']

I get a couple of errors further down, but these are from missing local versions of python (I only have 1 on my local machine, but have the other versions configured so that the CI runs them all), and two additional errors on the docs and report, which I guess is expected since the command is passing something specific to tox.

I will fix the pull request

ionelmc commented 2 years ago

Note that still can break if quotes are in the path. Also see https://github.com/tox-dev/tox/issues/924

Did you try this?

commands =
    check-manifest .
cbeiraod commented 2 years ago

Hi,

The check-manifest . approach also works fine. Let me know which approach you prefer and I can fix the PR if needed.

ionelmc commented 2 years ago

Let go with that (check-manifest .) instead, it's the least problematic.