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

Requirements Refactor (ready to merge, after conflict resolution) #1962

Merged
merged 17 commits into from
May 24, 2018

Conversation

techalchemy
Copy link
Member

Full refactor of requirements parsing

  • See commits

@@ -108,7 +109,7 @@ def test_convert_from_pip(expected, requirement):
),
])
def test_convert_from_pip_vcs_with_extra(expected, requirement):
assert pipenv.utils.convert_deps_from_pip(requirement) == expected
assert pipenv.requirements.PipenvRequirement.from_line(requirement).as_pipfile() == expected
Copy link
Member

Choose a reason for hiding this comment

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

You can merge this feature into DEP_PIP_PAIRS to make sure the new parser can catch the extra part.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure I'm following -- all tests are passing

Copy link
Member

Choose a reason for hiding this comment

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

nvm I’ll just push the change here.

@altendky
Copy link
Contributor

@techalchemy, thanks for the help on IRC. This seems to be working to solve my issue (local -e . install causing remote vcs link in Pipfile.lock causing a request that failed due to being a private repo and pip not being able to get the password from me... phew). Now I can get started writing my tests. :] maybe...

@techalchemy
Copy link
Member Author

@ncoghlan I've been sitting on this refactor for awhile, to summarize it essentially provides a compatibility layer between pipenv/pipfile style requirements and pip-style requirements. I wrote it in frustration when I had to fix a bug with the current codebase which breaks anytime someone touches it.

I've been contemplating breaking this into a library of its own, which I did with the last thing I had an idea like this for (https://github.com/techalchemy/pythonfinder) to allow for potential expansion independent of pipenv -- the API looks something like this:

>>> req = PipenvRequirement.from_line(requirement)
>>> req.as_pipfile()
{'req': {'key': 'value'}}

>>> req = PipenvRequirement.from_pipfile(name, index, {'key': 'value', 'key2': 'value2'})
>>> req.as_line()
-e asdf.whatever#egg=name

The object itself offers access to hashes, index, name, vcs, etc just depending on what type of requirement you pass. Check out requirements.py if you want, review if you have time, but general thoughts always appreciated

Will be around on synchronous communication for another hour or two => slack/irc/fb if you have anything general

@ncoghlan
Copy link
Member

@techalchemy Breaking out low level building blocks to make them easier to compose independently seems like a good idea to me. (As one example: it would be cool if pip-compile could accept Pipfile as input directly, and this would be a useful stepping stone towards enabling that)

@techalchemy techalchemy force-pushed the feature/requirements-refactor branch 4 times, most recently from 00e932a to cb156b9 Compare May 17, 2018 01:47
@kennethreitz
Copy link
Contributor

@techalchemy you have the authority to merge your own PRs

@kennethreitz
Copy link
Contributor

Please resolve merge conflicts.

@kennethreitz kennethreitz changed the title Requirements Refactor Requirements Refactor (ready to merge, after conflict resolution) May 23, 2018
Signed-off-by: Dan Ryan <[email protected]>
@techalchemy techalchemy force-pushed the feature/requirements-refactor branch from 95e850e to c154350 Compare May 24, 2018 02:04
@techalchemy techalchemy merged commit e41c065 into pypa:master May 24, 2018
@techalchemy techalchemy deleted the feature/requirements-refactor branch May 24, 2018 04:41
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.

5 participants