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 ignore flag #95

Merged
merged 1 commit into from
Mar 25, 2024
Merged

Conversation

mickvangelderen
Copy link
Contributor

@mickvangelderen mickvangelderen commented Nov 5, 2023

Add the ignore flag which make cargo-machete respect .ignore and .gitignore files when searching for files.

I've tried to make minimal changes. I've also tried not to make any breaking changes. The behavior should only change when passing --ignore. No guarantee of correctness.

src/search_unused.rs Outdated Show resolved Hide resolved
@mickvangelderen mickvangelderen force-pushed the replace-walkdir-with-ignore branch from 9f8cda8 to 2346be1 Compare November 5, 2023 13:19
@IgnisDa
Copy link

IgnisDa commented Nov 5, 2023

I feel like this should be the default behavior. Maybe via a breaking change in a major release?

@mickvangelderen
Copy link
Contributor Author

I'd say that doing so is a separate issue. I wouldn't want to block this capability on the discussion about whether it is the right time to do a major release, and what needs to go in that release. However, this is not up to me. That is up to the maintainer.

@mickvangelderen
Copy link
Contributor Author

@bnjbvr pinging you in case you are not yet aware of this PR

@bnjbvr
Copy link
Owner

bnjbvr commented Nov 10, 2023

Howdy! Sorry for not answering earlier, happy to see a PR for this, thanks. It might take a bit of time for me to review, since I'm in the middle of some big personal changes, but j should be able to get to it in the next week or so. In any case if someone from the community wants to review/test etc, feel free to do so!

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.

Can you add two tests please:

  • one where there's an ignored file that's filtered out because the ignore flag is set?
  • one where the ignore flag isn't set and machete successfully analyzes a crate that's in a directory that would be ignored if the flag was set?

@mickvangelderen
Copy link
Contributor Author

mickvangelderen commented Nov 10, 2023

I'm in the middle of some big personal changes

Good luck with that!

Can you add two tests please:

  • one where there's an ignored file that's filtered out because the ignore flag is set?

I believe this is covered by these lines: https://github.com/bnjbvr/cargo-machete/pull/95/files#diff-42cb6807ad74b3e201c5a7ca98b911c5fa08380e942be6e4ac5807f8377f87fcR305-R310. If skip_target_dir is false and respect_ignore_files is true, then no manifests are found because /target is in the .gitignore.

  • one where the ignore flag isn't set and machete successfully analyzes a crate that's in a directory that would be ignored if the flag was set?

I believe this is covered by https://github.com/bnjbvr/cargo-machete/pull/95/files#diff-42cb6807ad74b3e201c5a7ca98b911c5fa08380e942be6e4ac5807f8377f87fcR312-R317. If both skip_target_dir and respect_ignore_files are false, then at least one manifest is found.

I can split the test function into multiple test functions. I have not done so because the original already contained two cases. I am happy to refactor things a bit, but I'd like your approval before doing so. The PR will become a bit larger and require more effort to review properly. I can also do a follow-up PR. In fact I have some changes ready here.

@bnjbvr
Copy link
Owner

bnjbvr commented Feb 21, 2024

(Oops, sorry I've forgotten about this PR, I'll try to get back to it soon™)

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.

Sorry about the very long review time. The change looks great, I'll happily merge once the comments have been addressed. Thanks for the new tests too!

src/main.rs Show resolved Hide resolved
src/search_unused.rs Outdated Show resolved Hide resolved
@mickvangelderen mickvangelderen force-pushed the replace-walkdir-with-ignore branch 2 times, most recently from 52a942a to 87754d4 Compare March 25, 2024 11:39
Add the ignore flag which make cargo-machete respect .ignore and
.gitignore files when searching for files.
@mickvangelderen mickvangelderen force-pushed the replace-walkdir-with-ignore branch from 87754d4 to 01e1f7c Compare March 25, 2024 11:40
@mickvangelderen mickvangelderen requested a review from bnjbvr March 25, 2024 11:41
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!

@bnjbvr bnjbvr merged commit 2c20b2e into bnjbvr:main Mar 25, 2024
3 checks passed
@mickvangelderen mickvangelderen deleted the replace-walkdir-with-ignore branch March 25, 2024 12:00
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.

3 participants