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

Refactor CLI to lean more heavily on click's built in functionality #2814

Merged
merged 23 commits into from
Sep 5, 2018

Conversation

techalchemy
Copy link
Member

We currently handle a lot of parsing in custom functions that are quite broadly speaking broken at a lot of edge cases. This refactor (which I put off a lot because I didn't really understand how to do this with click's tooling, but now I do) restructures a lot of the cruft we've merged in over the last year or so and builds it back into click's parser.

Additionally this will centralize various options to avoid having them repeated throughout the CLI declarations. This also pulls in updates to requirementslib with some bugfixes and enhancements around VCS checkout handling, plus a minor tweak to our patched version of pip-tools which fixes a bug in some overlooked setup.py runners where we forgot to actually assign the resulting distribution to a variable (which has caused a number of issues).

Caught a few other very minor issues in the refactor during syntax cleanups.

- Allow parsing of multiple packages, editable and non-editable, interspersed
- Handle `--index` and `--extra-index-url` with click's native parsing
- Split commonly used options out into separate groups
- Handle packages and editable packages separately
- Allow `pip_install` to take `Requirement` objects as arguments rather than re-parsing
- Remove bad parsing logic
- Handle editable installs more effectively
- Actually store the distribution after we create it
- Actually get the dependencies from it
Signed-off-by: Dan Ryan <[email protected]>
Signed-off-by: Dan Ryan <[email protected]>
Signed-off-by: Dan Ryan <[email protected]>
Signed-off-by: Dan Ryan <[email protected]>

Update requirementslib

Signed-off-by: Dan Ryan <[email protected]>
Signed-off-by: Dan Ryan <[email protected]>

Don't re-clone repos now that this works

Signed-off-by: Dan Ryan <[email protected]>

Prune peeps directory from manifest

Signed-off-by: Dan Ryan <[email protected]>

Fix nonetype uris

Signed-off-by: Dan Ryan <[email protected]>

Manually lock requirements?

Signed-off-by: Dan Ryan <[email protected]>

Update requirementslib - leave context before updating sha

Signed-off-by: Dan Ryan <[email protected]>

Fix requirementslib vcs checkouts

Signed-off-by: Dan Ryan <[email protected]>

fix tmpdir implementation

Signed-off-by: Dan Ryan <[email protected]>

final fix for vcs uris

Signed-off-by: Dan Ryan <[email protected]>

Allow for adding requirements objects directly to pipfile

Signed-off-by: Dan Ryan <[email protected]>

Update piptools patch

Signed-off-by: Dan Ryan <[email protected]>
@techalchemy techalchemy changed the title WIP: Refactor CLI to lean more heavily on click's built in functionality Refactor CLI to lean more heavily on click's built in functionality Sep 4, 2018
@techalchemy techalchemy added Type: Enhancement 💡 This is a feature or enhancement request. PR: awaiting-merge The PR related to this issue has been reviewed and is awaiting merge. Type: Vendored Dependencies This issue affects vendored dependencies within pipenv. labels Sep 4, 2018
def callback(ctx, param, value):
state = ctx.ensure_object(State)
if isinstance(value, (tuple, list)):
state.extra_index_urls.extend(list(value))
Copy link
Member

Choose a reason for hiding this comment

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

No need to convert this one extra time, extend() works for any iterables. But is this check (to distinguish input type from strings) needed at all? Doesn’t the multiple=True flag guarantee this always gets a list?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm I think so, will fix

return value
return option("--python", default=False, nargs=1, callback=callback,
help="Specify which version of Python virtualenv should use.",
expose_value=False)(f)
Copy link
Member

Choose a reason for hiding this comment

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

I hope we can merge two, three and python soon, but it probably needs to be done after this is merged.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did consider that, but actually they are grouped together in common_options and don't appear anywhere else anyway. I tried my best to keep this organized and grouped based on how we are using these things currently. The real concern here (and the thing to watch out for in this review) is that the state passing approach I think will pass the cli options and args as well... so we just need to be sure this doesn't accidentally add new CLI options to the API

@kennethreitz
Copy link
Contributor

@uranusjr i personally don't think combining two/three and python are a good idea, but as two is slowly dying away, it may be a good idea. In either case, it will require a PEEP.

@uranusjr
Copy link
Member

uranusjr commented Sep 4, 2018

@kennethreitz I don’t mean combining them in the CLI, but that --two/--three/--python can be parsed into one single argument to be passed into functions in core. This is already done when those values are used, I only propose moving the logic earlier.

@kennethreitz
Copy link
Contributor

ah, gotcha!

@uranusjr uranusjr merged commit b771621 into master Sep 5, 2018
@uranusjr uranusjr deleted the fix-cli branch September 5, 2018 04:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment