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.

Sign up for GitHub

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

Jump to bottom

Added nested json output to pipenv graph command #2199

Merged
merged 5 commits into from
May 17, 2018
Merged

Added nested json output to pipenv graph command #2199

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0479d84
Added nested json output form pipdeptree to graph command
PieterjanMontens May 15, 2018
bc4bf56
Merge branch 'master' into feature/graph-nested-json
PieterjanMontens May 16, 2018
9350939
Merge branch 'master' into feature/graph-nested-json
kennethreitz May 16, 2018
a6611d2
Merge branch 'master' into feature/graph-nested-json
kennethreitz May 16, 2018
70a7b2b
Merge branch 'master' into feature/graph-nested-json
kennethreitz May 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions pipenv/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -841,13 +841,14 @@ def update(
)
@option('--bare', is_flag=True, default=False, help="Minimal output.")
@option('--json', is_flag=True, default=False, help="Output JSON.")
@option('--json-tree', is_flag=True, default=False, help="Output JSON in nested tree.")
@option(
'--reverse', is_flag=True, default=False, help="Reversed dependency graph."
)
def graph(bare=False, json=False, reverse=False):
def graph(bare=False, json=False, json_tree=False, reverse=False):
from .core import do_graph

do_graph(bare=bare, json=json, reverse=reverse)
do_graph(bare=bare, json=json, json_tree=json_tree, reverse=reverse)


@command(short_help="View a given module in your editor.", name="open")
Expand Down
34 changes: 33 additions & 1 deletion pipenv/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2409,7 +2409,7 @@ def do_check(three=None, python=False, system=False, unused=False, args=None):
sys.exit(1)


def do_graph(bare=False, json=False, reverse=False):
def do_graph(bare=False, json=False, json_tree=False, reverse=False):
import pipdeptree
try:
python_path = which('python')
Expand All @@ -2433,9 +2433,31 @@ def do_graph(bare=False, json=False, reverse=False):
err=True,
)
sys.exit(1)
if reverse and json_tree:
click.echo(
u'{0}: {1}'.format(
crayons.red('Warning', bold=True),
u'Using both --reverse and --json-tree together is not supported. '
u'Please select one of the two options.',
),
err=True,
)
sys.exit(1)
if json and json_tree:
click.echo(
u'{0}: {1}'.format(
crayons.red('Warning', bold=True),
u'Using both --json and --json-tree together is not supported. '
u'Please select one of the two options.',
),
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 •
edited
Loading

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 👍

flag = ''
if json:
flag = '--json'
if json_tree:
flag = '--json-tree'
if reverse:
flag = '--reverse'
if not project.virtualenv_exists:
Expand Down Expand Up @@ -2465,6 +2487,16 @@ def do_graph(bare=False, json=False, reverse=False):
data.append(d)
click.echo(simplejson.dumps(data, indent=4))
sys.exit(0)
elif json_tree:
def traverse(obj):
if isinstance(obj, list):
return [traverse(package) for package in obj if package['key'] not in BAD_PACKAGES]
else:
obj['dependencies'] = traverse(obj['dependencies'])
return obj
data = traverse(simplejson.loads(c.out))
click.echo(simplejson.dumps(data, indent=4))
sys.exit(0)
else:
for line in c.out.split('\n'):
# Ignore bad packages as top level.
Expand Down
1 change: 1 addition & 0 deletions tests/integration/test_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ def test_pipenv_graph(PipenvInstance, pypi):
p.pipenv('install requests')
assert 'requests' in p.pipenv('graph').out
assert 'requests' in p.pipenv('graph --json').out
assert 'requests' in p.pipenv('graph --json-tree').out


@pytest.mark.cli
Expand Down