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

Added nested json output to pipenv graph command #2199

Merged
merged 5 commits into from
May 17, 2018

Conversation

PieterjanMontens
Copy link
Contributor

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)
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@PieterjanMontens PieterjanMontens May 16, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ;)

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, great 👍

@kennethreitz
Copy link
Contributor

simplicity is better than functionality.

@PieterjanMontens
Copy link
Contributor Author

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
@PieterjanMontens PieterjanMontens deleted the feature/graph-nested-json branch May 17, 2018 08:57
@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
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants