Hi,

First of all, sorry about the delay in looking at the PR.

I didn't understand the need for all, none and selection choices for the --extra argument. In fact, I am not sure of the use case for having the --extra argument at all. IMO, pipdeptree should always consider extra requirements (at present it's a bug that it can't link extra deps). Having said that, if you have a use case, please let me know.

I think without the --extra option, the implementation can be far simpler:

1. Implement DistPackage.extra_requires which will return just the package keys by parsing package metadata
2. In PackageDAG.from_pkgs we can additionally call p.extra_requires, then look up the actual package object for the key from idx variable and call .as_requirement() method on it to get a ReqPackage object for it.
3. In ReqPackage, we may introduce a new attribute is_extra to indicate that it's an extra requirement. It can be used in the presentation logic.

    @classmethod
    def from_pkgs(cls, pkgs):
        pkgs = [DistPackage(p) for p in pkgs]
        idx = {p.key: p for p in pkgs}
        m = {p: [ReqPackage(r, idx.get(r.key))  for r in p.requires()] + [idx[e.key].as_requirement(extra=True) for e in p.extra_requires()]
                 for p in pkgs}
        return cls(m)

This is quite similar to your implementation but there's no extra list to filter from. The extra=True argument to .as_requirement can be passed when initializing ReqPackage in DistPackage.as_requirement method. The flag can be used in the presentation logic (render_text or dump_graphviz functions).

I haven't tried this actually but the way I see it is as follows -

Currently pipdeptree cannot identify extra requirements For eg. if oauthlib[rsa] is installed in the environment, then pipdeptree outputs oauthlib and cryptography as packages but cryptography is not considered a dependency of oauthlib. If we resolve this ourselves in PackageDAG.from_pkgs, then everything else should just work. I think the all and selection options should be automatically taken care of by the existing --all and --package/--exclude.

Hi,
thanks for your reply.
The idea behind the --extra argument was that quite a few packages have a lot of extra requirements (e.g. testing, etc.) and all those extra requirements made the graphs or stdout quite unreadable. And I realized only plotting/printing those of the selection (through the -p option) made it clearer. Now if you do not think this is useful, I can take it out.
I'll have a look at your propositions about the implementation and come back to you.
[edited] I just realized your last line. That's a good point. To have the option taken care of automatically by the existing ones.

I don't understand why you differentiate: ReqPackage(r, idx.get(r.key)) and idx[e.key].as_requirement()? Should they not use the same initiation?

The idea behind the --extra argument was that quite a few packages have a lot of extra requirements (e.g. testing, etc.) and all those extra requirements made the graphs or stdout quite unreadable. And I realized only plotting/printing those of the selection (through the -p option) made it clearer

yes, but won't the -p arg take care of that? For eg. I have installed oauthlib[rsa] and ipython in a virtualenv to test this case. oauthlib[rsa] installs the extra dependency cryptography. So,

• pipdeptree should show cryptography as a dependency of oauthlib (no -p arg)
• pipdeptree -p oauthlib should also show cryptography as a dependency of oauthlib.
• But pipdeptree -p ipython won't show cryptography or oauthlib

Am I missing anything?

I don't understand why you differentiate: ReqPackage(r, idx.get(r.key)) and idx[e.key].as_requirement()? Should they not use the same initiation?

There's no difference actually. But with idx[e.key].as_requirement(), there's no need to explicitly call pkg_resources.Requirement which you're doing in the extra_requires method.

Now I understand the confusion: your solution would only retain the extra dependencies that are installed on the system/virtualenv. The way I did it, all extra deps are mapped (even if not installed). I thought it'd be nice to see what is installed with which option, as prospecting if you wish. Do you think it's a bad idea?

The reason why I wrote an extra property and not just is_extra (boolean) is because the extra dependency has a name different than the parent or the dependency. E.g. wheel: [test] pytest, where test is the installation option and pytest the name of the dependency.

As for the explicit call pkg_resources.Requirement in the extra_requires method, this has to be so for the version information. E.g. for the same example as above, the way I wrote the metadata parser, it would return ('test', 'pytest (>=3.0.0)'). Creating a Requirement instance from the second string in the parenthesis gives you package name and version information.

Now I understand the confusion: your solution would only retain the extra dependencies that are installed on the system/virtualenv. The way I did it, all extra deps are mapped (even if not installed). I thought it'd be nice to see what is installed with which option, as prospecting if you wish. Do you think it's a bad idea?

IMO, it's outside the scope of pipdeptree which is basically limited to only packages that are installed on the system/virtualenv. Not sure about the use case, but it could be a separate tool that provides this functionality.

The reason why I wrote an extra property and not just is_extra (boolean) is because the extra dependency has a name different than the parent or the dependency. E.g. wheel: [test] pytest, where test is the installation option and pytest the name of the dependency.

Got it

As for the explicit call pkg_resources.Requirement in the extra_requires method, this has to be so for the version information. E.g. for the same example as above, the way I wrote the metadata parser, it would return ('test', 'pytest (>=3.0.0)'). Creating a Requirement instance from the second string in the parenthesis gives you package name and version information.

Makes sense.

Also, can you please also provide the pip freeze output of the virtualenv where you're testing this change? I can test it against the same env.

Alright, I'll leave the external packages out. Here's the pip freeze:

apipkg==1.5
appdirs==1.4.4
attrs==20.1.0
contextlib2==0.6.0.post1
coverage==5.3
distlib==0.3.1
execnet==1.7.1
filelock==3.0.12
flake8==3.8.4
flake8-2020==1.6.0
graphviz==0.14.1
importlib-metadata==1.7.0
importlib-resources==3.0.0
iniconfig==1.0.1
jaraco.envs==2.0.0
mccabe==0.6.1
mock==4.0.2
more-itertools==8.5.0
packaging==20.4
path==15.0.0
path.py==12.5.0
Paver==1.3.4
pkg-resources==0.0.0
pluggy==0.13.1
py==1.9.0
pycodestyle==2.6.0
pyflakes==2.2.0
pyparsing==2.4.7
pytest==6.0.1
pytest-cov==2.10.1
pytest-fixture-config==1.7.0
pytest-flake8==1.0.6
pytest-shutil==1.7.0
pytest-virtualenv==1.7.0
six==1.15.0
termcolor==1.1.0
toml==0.10.1
tox==3.20.0
tox-venv==0.4.0
virtualenv==20.0.33
zipp==3.1.0

(pipdeptree, I installed with -e)

[Edited] I also installed setuptools[tests] in order to have some extra deps installed

I'm sorry but I can't seem to find where test_custom_interpreter is defined

I'm sorry but I can't seem to find where test_custom_interpreter is defined

It was introduced in a recently merged PR #128. I think you rebased your branch with latest master?

Ah yes, thanks. I didn't know github performed auto rebase on the pull request.

Alright, ready to merge on my side. Let me know if I can still do anything

Hi,

Thanks for your work on this long pending feature/bug. I wanted to suggest some refactoring but was finding it difficult to communicate exactly. So, I've cherry-picked your commits on another branch and added some more changes on top - #138. Still need to get the tests passing.

Closing this PR. It will be merged via PR #138

Thanks

Alright, perfect. Thank you too for your comments and for finishing the PR.