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 parsing of editable packages, fixes #2714 #2733

Closed
wants to merge 3 commits into from

Conversation

slhck
Copy link
Contributor

@slhck slhck commented Aug 13, 2018

Fix the parsing of an editable package. When the package name is '-e', it will
be concatenated with the next package in the list (which is the VCS URL).

The issue

This fixes parsing of editable installations, see issue #2714.

It allows running:

$ pipenv install -e "<vcs-url>"
The fix

The problem is that in core.py, line number 1767, the value of line is:

"-e git+ssh://…"

And then package_names = line.split() is called, which again makes the first package name -e, which leads to the error.

The checklist
  • Associated issue
  • A news fragment in the news/ directory to describe this fix with the extension .bugfix, .feature, .behavior, .doc. .vendor. or .trivial (this will appear in the release changelog). Use semantic line breaks and name the file after the issue number or the PR #.

Fix the parsing of an editable package. When the package name is '-e', it will
be concatenated with the next package in the list (which is the VCS URL).
@slhck
Copy link
Contributor Author

slhck commented Aug 13, 2018

I guess that this still only works if the first package is to be installed in editable mode. Any subsequent option -e would be considered a separate package again.

I would probably look into rewriting the parser for package names such that it collects the options one-by-one, as the splitting based on spaces seems to be fragile here. I can help with that, or is there any open PR regarding this already?

@techalchemy
Copy link
Member

I think we’d like to move away from parsing things the way we currently are and instead we would like to use clicks parser for this (and other) options — been meaning to get to that myself but if you want to take a look I’m happy to review a PR as well

@techalchemy
Copy link
Member

(This is definitely a bit fragile as it comes in through cli.py and gets parsed possibly multiple times through its time moving through the code base)

@slhck
Copy link
Contributor Author

slhck commented Aug 13, 2018

I understand why you'd want to move away from this kind of parsing! It's definitely not very stable or maintenance-friendly. It's the first time I've looked into the code base and I only found this part of the code in core.py – is there any other part that would need to be adapted? AFAICT this is the only time where a user-entered specification is parsed.

I think until a major rewrite is available, this fix would immerdiately take care of the one issue I found. Or would you hold off from merging this for any particular reason? I'm happy to improve this PR.

@techalchemy
Copy link
Member

I am guessing this will work -- I'd settle for an isolated argument parser even, not sure what @uranusjr thinks of that but if not we can go ahead with this for now

(We are actually in the process of a big rewrite of several components, which is why we have been sort of slow responding on here)

@slhck
Copy link
Contributor Author

slhck commented Aug 13, 2018

No problem, I appreciate your feedback! Take all the time you need. (Just a happy pipenv user trying to fix the one thing that's bugging him almost daily :) – I can work off my branch in the meantime.)

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