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(packages/dependency): add space after filename for file dependencies with markers #153

Merged
merged 2 commits into from
Apr 26, 2021

Conversation

ossareh
Copy link
Contributor

@ossareh ossareh commented Apr 1, 2021

Resolves: python-poetry/poetry#3872

  • Added tests for changed code.
  • Updated documentation for changed code.

local vendored files need a space after the file name and before the ";" which demarks the start of
markers

@ossareh
Copy link
Contributor Author

ossareh commented Apr 1, 2021

Any advice on how to fix the windows related issues would be very much appreciated. I understand the nature of why the tests would fail in this way, but I'm not at all sure what the right way to handle this is.

I have now appeased the windows 😅

@ossareh ossareh force-pushed the fix/space_after_file_path branch 2 times, most recently from acd21af to 2e98b2e Compare April 1, 2021 21:22
@ossareh
Copy link
Contributor Author

ossareh commented Apr 1, 2021

Update: this solves the case of the missing space prior to the ";", but immediately runs up against the fact that a relative file is not a valid URL:

...snip...
  File "/home/ossareh/dev/src/github.com/erisyon/vega/.tox/py39/lib/python3.9/site-packages/pip/_vendor/pkg_resources/__init__.py", line 3033, in _compute_dependencies
    reqs.extend(parse_requirements(req))
  File "/home/ossareh/dev/src/github.com/erisyon/vega/.tox/py39/lib/python3.9/site-packages/pip/_vendor/pkg_resources/__init__.py", line 3094, in parse_requirements
    yield Requirement(line)
  File "/home/ossareh/dev/src/github.com/erisyon/vega/.tox/py39/lib/python3.9/site-packages/pip/_vendor/pkg_resources/__init__.py", line 3103, in __init__
    raise RequirementParseError(str(e))
pip._vendor.pkg_resources.RequirementParseError: Invalid URL: blob/pyvcam-2.1.0-cp39-cp39-linux_x86_64.whl

It's possible that my "fix" is actually fixing the wrong thing; if I was including this as an absolute path with a file:// scheme the is_url() portion of my change may actually just do the right thing.

In my case I can't provide an absolute path due to varied setup on hosts that'll receive this code.

I'm going to think about this some more, if you decide the close this PR without merging I understand :) - if you want changes made, I'm happy to make them.

@ossareh
Copy link
Contributor Author

ossareh commented Apr 2, 2021

It's possible that my "fix" is actually fixing the wrong thing; if I was including this as an absolute path with a file:// scheme the is_url() portion of my change may actually just do the right thing.

after more research I see this is incorrect.

This change makes the output of this stage of the process 508 compliant, where as prior to this it was not. So in that regard this is indeed fixing a problem.

The fact that pip doesn't have a solution for understanding relative paths is a well discussed situation: pypa/pip#6658

Copy link
Member

@finswimmer finswimmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot. Looks good to me 👍

Just a small suggestion to improve the test.

@@ -62,3 +65,31 @@ def test_file_dependency_pep_508_local_file_relative_path(mocker):

requirement = "{} @ {}".format("demo", path)
_test_file_dependency_pep_508(mocker, "demo", path, requirement)


def test_to_pep_508_with_marker(mocker):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can use a parameterize test here, to test absolute and relative path.

…ies with markers

local vendored files need a space after the file name and before the ";" which demarks the start of
markers

fix #3872
@ossareh
Copy link
Contributor Author

ossareh commented Apr 25, 2021

@finswimmer thanks for the feedback 👍

I went ahead and restructured that unit test into two. I considered parameterizing the test and I could see three main options:

  1. a loop which calls the underlying test with an absolute path and a relative path
  2. calling a new _test_to_508_with_marker with the paths and expectation which then calls _test_file_dependency_pep_508
  3. splitting the unit test into two for each case

I felt the 3rd option strikes the best balance with clarity, it's a step necessary for the 2nd option, and I'm happy to add that additional layer if you think the deduplication is desirable.

@finswimmer finswimmer merged commit fe476e0 into python-poetry:master Apr 26, 2021
@sdispater sdispater mentioned this pull request Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants