search

jplatte / cargo-depgraph

Creates graphviz dependency graphs for Rust projects that use Cargo
GNU General Public License v3.0
151 stars 8 forks source link

Add `--depth` option #19

Closed BennD closed 9 months ago

BennD commented 9 months ago

added --depth option removed --direct-dependencies-only option, as it is equivalent to --depth 1

BennD commented 9 months ago

My new version of --depth works with a depth of 0 now and also works more like --workspace-only. I removed the is_workspace_member check, as only non-workspace crates end up in the HashMapEntry::Vacant(v) => {...} pattern. Workspace crates are all already added in line 48.

To simplify the code and reduce redundancy, we could remove the --workspace_only check and instead set --depth to 0 if the --workspace-only option is used. In case --workspace-only and --depth are defined, --workspace-only should override whatever is set with --depth. Maybe issue a warning though? Or just fail and tell the user that those options are not compatible?

jplatte commented 9 months ago

Let's make them mutually exclusive. I think clap supports mutually exclusive arguments somehow (you don't even have to raise an error "by hand").

jplatte commented 9 months ago

Re. removal of is_workspace_member, are you sure about that? I think if the user explicitly sets a root package, workspace members aren't all added at the same time.

BennD commented 9 months ago

Re. removal of is_workspace_member, are you sure about that? I think if the user explicitly sets a root package, workspace members aren't all added at the same time.

You are correct. That also means --workspace-only is not equal to --depth 0 as initially thought.

jplatte commented 9 months ago

Oh, as in --workspace-only combined with --root foo is different? That is true, maybe that's a reason to keep both flags, not just for now but indefinitely.

BennD commented 9 months ago

Oh, as in --workspace-only combined with --root foo is different? That is true, maybe that's a reason to keep both flags, not just for now but indefinitely.

Yes. Though i don't see a real usecase of the top of my head, you could even combine it further with --depth, so i would keep it like it is now and not make it mutually exclusive.