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

Add --depth option #19

Merged
merged 4 commits into from
Dec 7, 2023
Merged

Add --depth option #19

merged 4 commits into from
Dec 7, 2023

Conversation

BennD
Copy link
Contributor

@BennD BennD commented Dec 5, 2023

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

removed `--direct-dependencies-only` option, as it is equivalent to `--depth 1`
@jplatte jplatte changed the title #18 added --depth option Add --depth option Dec 5, 2023
Comment on lines 102 to 105
// Don't add dependencies of dependencies if we're at the depth limit
if depth + 1 < config.depth.unwrap_or(u32::MAX) {
deps_add_queue.push_back((dep.pkg.clone(), depth + 1));
}
Copy link
Owner

Choose a reason for hiding this comment

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

Thinking more about this, workspace-only seems like a special case of depth. I don't think it should be removed here in favor of --depth=0, but I wonder why workspace-only is handled differently than this. If you don't have an answer that's fine, I guess that means I should just revisit this code after merging the PR but before releasing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While thinking about this i noticed that --depth 0 is actually broken.
I will take a look at this either tomorrow or the day after.

Copy link
Owner

Choose a reason for hiding this comment

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

Great, thanks :)

simplified workspace member detection
@BennD
Copy link
Contributor Author

BennD commented Dec 6, 2023

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
Copy link
Owner

jplatte commented Dec 7, 2023

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
Copy link
Owner

jplatte commented Dec 7, 2023

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
Copy link
Contributor Author

BennD commented Dec 7, 2023

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
Copy link
Owner

jplatte commented Dec 7, 2023

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
Copy link
Contributor Author

BennD commented Dec 7, 2023

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.

src/graph/build.rs Outdated Show resolved Hide resolved
Co-authored-by: Jonas Platte <jplatte+git@posteo.de>
Copy link
Owner

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

Thanks!

@jplatte jplatte merged commit fde6d8d into jplatte:main Dec 7, 2023
@jplatte jplatte mentioned this pull request Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants