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
Member

techalchemy commented Sep 3, 2018

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.

techalchemy added some commits Aug 31, 2018

Refactor CLI options
- 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
Clean up core arguments
- Handle packages and editable packages separately
- Allow `pip_install` to take `Requirement` objects as arguments rather than re-parsing
- Remove bad parsing logic
Update piptools patch
- Handle editable installs more effectively
- Actually store the distribution after we create it
- Actually get the dependencies from it
Refactor CLI for organization and simplicity
Signed-off-by: Dan Ryan <dan@danryan.co>
Cleanup unicode literals warnings
Signed-off-by: Dan Ryan <dan@danryan.co>
Fix some typos
Signed-off-by: Dan Ryan <dan@danryan.co>
Clean up vcs ref checkouts
Signed-off-by: Dan Ryan <dan@danryan.co>
Update requirementslib
Signed-off-by: Dan Ryan <dan@danryan.co>
Fix `lock -r` output to include all markers
- Fixes #2748

Signed-off-by: Dan Ryan <dan@danryan.co>
Turn off no-deps for tarballs/zips
- Fixes #2173

Signed-off-by: Dan Ryan <dan@danryan.co>
Reorganize pip installation to ditch custom parser
Signed-off-by: Dan Ryan <dan@danryan.co>
Fix installation of multiple packages
Signed-off-by: Dan Ryan <dan@danryan.co>
Minor project fixes
Signed-off-by: Dan Ryan <dan@danryan.co>
Add missing skip_lock option
Signed-off-by: Dan Ryan <dan@danryan.co>
Fix editor auto-deletions of piptools patch lines
Signed-off-by: Dan Ryan <dan@danryan.co>
Syntax error...
Signed-off-by: Dan Ryan <dan@danryan.co>

Update requirementslib

Signed-off-by: Dan Ryan <dan@danryan.co>
Update requirementslib and fix VCS installation
Signed-off-by: Dan Ryan <dan@danryan.co>

Don't re-clone repos now that this works

Signed-off-by: Dan Ryan <dan@danryan.co>

Prune peeps directory from manifest

Signed-off-by: Dan Ryan <dan@danryan.co>

Fix nonetype uris

Signed-off-by: Dan Ryan <dan@danryan.co>

Manually lock requirements?

Signed-off-by: Dan Ryan <dan@danryan.co>

Update requirementslib - leave context before updating sha

Signed-off-by: Dan Ryan <dan@danryan.co>

Fix requirementslib vcs checkouts

Signed-off-by: Dan Ryan <dan@danryan.co>

fix tmpdir implementation

Signed-off-by: Dan Ryan <dan@danryan.co>

final fix for vcs uris

Signed-off-by: Dan Ryan <dan@danryan.co>

Allow for adding requirements objects directly to pipfile

Signed-off-by: Dan Ryan <dan@danryan.co>

Update piptools patch

Signed-off-by: Dan Ryan <dan@danryan.co>

@techalchemy techalchemy force-pushed the fix-cli branch from e484256 to b33dfa6 Sep 4, 2018

Windows edge case and news
Signed-off-by: Dan Ryan <dan@danryan.co>

@techalchemy techalchemy changed the title from WIP: Refactor CLI to lean more heavily on click's built in functionality to Refactor CLI to lean more heavily on click's built in functionality Sep 4, 2018

@techalchemy techalchemy added this to In progress in Consolidate and Refactor CLI Code via automation 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))

This comment has been minimized.

@uranusjr

uranusjr Sep 4, 2018

Member

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?

This comment has been minimized.

@techalchemy

techalchemy Sep 4, 2018

Member

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)

This comment has been minimized.

@uranusjr

uranusjr Sep 4, 2018

Member

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

This comment has been minimized.

@techalchemy

techalchemy Sep 4, 2018

Member

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

This comment has been minimized.

Contributor

kennethreitz commented Sep 4, 2018

@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

This comment has been minimized.

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

This comment has been minimized.

Contributor

kennethreitz commented Sep 4, 2018

ah, gotcha!

techalchemy added some commits Sep 4, 2018

Fix option in cli and get released requirementslib
Signed-off-by: Dan Ryan <dan@danryan.co>

@uranusjr uranusjr merged commit b771621 into master Sep 5, 2018

3 checks passed

buildkite/pipenv Build #1336 passed (11 minutes)
Details
pipenv CI (Linux) #20180904.21 succeeded
Details
pipenv CI (Windows) #20180904.7 succeeded
Details

Consolidate and Refactor CLI Code automation moved this from In progress to Done Sep 5, 2018

@uranusjr uranusjr deleted the fix-cli branch Sep 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment