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

Added nested json output to pipenv graph command #2199

Merged
merged 5 commits into from May 17, 2018

Conversation

Projects
3 participants
@PieterjanMontens
Contributor

PieterjanMontens commented May 15, 2018

Hello all,

This is a small contribution to solve a particular need I had. I made the necessary changes (exposing pipdeptree's --json-tree*) in a fork (didn't have contributor status before, let me know if I should do a new PR from the base fork), filtered the BAD_PACKAGES from the output and added a small test, based on the work that had been done before.

  • pipenv's shipped version of pipdeptree has this feature
),
err=True,
)
sys.exit(1)

This comment has been minimized.

@uranusjr

uranusjr May 16, 2018

Member

I feel we should be more graceful here. There are two possible interfaces (taking Click’s limitations in mind):

  • Use the current options, but --json --json-tree simply means --json-tree.
  • Add a --type option (instead of --json-tree). --json --type=flat outputs the current --json result, which --json --type=true uses --json-tree. We can even implement a flat output for the non-JSON variant. If --type is not specified, the current behaviour is the default (flat for JSON, nested for non-JSON).

This comment has been minimized.

@PieterjanMontens

PieterjanMontens May 16, 2018

Contributor

There's indeed room to be more graceful, I considered that also (or even with--flat & --nested options), but opted for the --json-tree option because:

  • pipenv graph is mostly an interface to pipdeptree;
  • abstracting pipdeptree's arguments away would add a layer of confusion (in it's usage but also for developments down the road);
  • future versions of pipdeptree could add more arguments, which could conflict with pipenv's own graph commands;
  • it would mean more code will have to be maintained.

I also considered exposing --graph-output, but that didn't seem justified in a Dev & CI/CD environment.

This comment has been minimized.

@PieterjanMontens

PieterjanMontens May 16, 2018

Contributor

Another option, if Click's version accepts it, is to forward all unknown options to pipdeptree's wrapper, which would reduce the load of exposing it's functionalities, and facilitate integration of future versions of that package.

Edit: current included Click version is 6.7, so yes, it's a possibility

This comment has been minimized.

@uranusjr

uranusjr May 16, 2018

Member

Sounds reasonable to me. In this case, however, maybe it would be better to just hand the flags to pipdeptree without validation? pipdeptree would happily accept --json --json-tree --reverse, but just silently ignore some of them.

This comment has been minimized.

@PieterjanMontens

PieterjanMontens May 16, 2018

Contributor

That would be the case, yes. However, we can specify in the help text how the flags behave, and who overpowers whom.

closed->reopened->closed->reopened: whoopsy, is that usual ?

This comment has been minimized.

@PieterjanMontens

PieterjanMontens May 17, 2018

Contributor

Thinking of it, forwarding unknown options is not really desirable: it would mean the options wouldn't be defined in click, meaning they wouldn't appear in the --help, meaning more confusion and just plain obfuscation... let's not do that ;)

This comment has been minimized.

@uranusjr

uranusjr May 17, 2018

Member

Yeah I agree. Registering the options to Click is a good idea, the only doubt I have is whether we should perform additional checks. It’s really minor though, let’s just merge it and come back to this if someone complains.

This comment has been minimized.

@PieterjanMontens

PieterjanMontens May 17, 2018

Contributor

OK, great 👍

PieterjanMontens and others added some commits May 16, 2018

@kennethreitz

This comment has been minimized.

Contributor

kennethreitz commented May 16, 2018

simplicity is better than functionality.

@PieterjanMontens

This comment has been minimized.

Contributor

PieterjanMontens commented May 16, 2018

I like this quote, from Don Norman:

“The argument is not between adding features and simplicity, between adding capability and usability, The real issue is about design: designing things that have the power required for the job while maintaining understandability, the feeling of control, and the pleasure of accomplishment.”

@uranusjr uranusjr merged commit 85c641d into pypa:master May 17, 2018

3 of 4 checks passed

buildkite/pipenv/python Started...
Details
buildkite/pipenv Build #101 passed (5 minutes, 31 seconds)
Details
buildkite/pipenv/pipeline Passed (3 seconds)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@PieterjanMontens PieterjanMontens deleted the PieterjanMontens:feature/graph-nested-json branch May 17, 2018

@techalchemy techalchemy moved this from Done to Needs Changelog in 2018.06.x Release Jun 16, 2018

@techalchemy techalchemy moved this from Needs Changelog to Done in 2018.06.x Release Jun 16, 2018

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