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

Ignore n levels instead of just leaves #54

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

incaseoftrouble
Copy link

WIP

I still have to actually test this but small bump to get some feedback :)

Fixes #51

(After this I can also implement #50 )

@blingenf
Copy link
Owner

blingenf commented Apr 5, 2024

Thank you! A few comments:

  • I think ignore_leaf will have to become ignore_depth on line 120 of __main__.py
  • for API changes (changing type to file_type and ignore_leaf to ignore_depth) I try to keep the old parameters working for one version with a DeprecationWarning. But I can add that myself after this is merged -- I think the parameter changes are sensible.
  • The new parameter should be documented in README.md and docs/cmdline.rst
  • Each parameter for the detector should have a relevant unit test in tests/test_detector.py

But in general where / how this is implemented looks good to me. Once it's tested and the unit tests are passing I should be good to merge.

@incaseoftrouble
Copy link
Author

incaseoftrouble commented Apr 6, 2024

Sure, didn't get to it this week :-)

I also added a get_comparison_pairs() to separate out the "ignore pair" logic -- this made it sensible to let the progress bar go over these pairs directly instead of test files (but this I can revert).

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.

Feature Request: Ignore n levels of "leafs"
2 participants