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

feat: remove features when removing dependencies #113

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

njelich
Copy link

@njelich njelich commented Mar 14, 2024

When removing dependencies using the --fix flag, the features for those dependencies are not removed. This adds support for that.

I tested it manually, but tests should probably be written to make sure it works generally. From what I can see there are currently no tests for the --fix functionality anyway.

@njelich
Copy link
Author

njelich commented Mar 16, 2024

@bnjbvr Pending review

@bnjbvr
Copy link
Owner

bnjbvr commented Mar 18, 2024

Hi, as I'm working on my spare time on this project, I might not be very reactive for reviews and all of that.

That being said: how does this work? How can you guess the feature's name? (are you assuming the feature's named after the dependency?) Not sure how prevalent that usage is, so unclear whether we should do that automatically...

@njelich
Copy link
Author

njelich commented Mar 18, 2024

@bnjbvr I added a test case so you can see what it does. You can also add more autofix tests using the same setup.

@njelich
Copy link
Author

njelich commented Mar 18, 2024

In short, for a Cargo.toml with the unused log dependency:

[dependencies]
log = "0.4.14"

[features]
default = ["std"]
std = ["log/std", "other"]
other = []

It sees the log dependency is unused, and that the std feature on the log dependency is used with log/std. When removing log, it also removes the log/std feature.

[dependencies]

[features]
default = ["std"]
std = [ "other"]
other = []

@njelich
Copy link
Author

njelich commented Mar 22, 2024

@bnjbvr Have you had a chance to take a look at this?

Copy link
Owner

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

Thanks, took the time to review.

Bonus points if you switch to cargo-insta for the snapshot tests :) (but not mandatory, of course!).

src/main.rs Outdated
if let Some(deps) = deps.as_array_mut() {
deps.retain(|dep| {
if let Some(dep) = dep.as_str() {
!dependencies_list.iter().any(|d| dep.contains(d))
Copy link
Owner

Choose a reason for hiding this comment

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

I think contains is a bit too vague here: a dependency flagged for removal called log would cause removal of a feature enabling something (a feature or another dependency) called flog.

Can you handle that better, and add a test that checks that too, please?

@bnjbvr bnjbvr self-requested a review August 23, 2024 09:40
@bnjbvr
Copy link
Owner

bnjbvr commented Aug 23, 2024

Getting back to this: any idea why the CI doesn't even show up? There's no job and no way for me to approve the run or not 🤔

Copy link
Owner

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

I don't know why the CI job fails, but when I tried to run the tests locally by repeating the commands, they failed. Could you take another look please?

@njelich
Copy link
Author

njelich commented Sep 2, 2024

Will check later today

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.

2 participants