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: add support for -I #24

Merged
merged 2 commits into from
Sep 9, 2024
Merged

feat: add support for -I #24

merged 2 commits into from
Sep 9, 2024

Conversation

ByteBaker
Copy link
Contributor

  • also add test case
  • remove fn remove_empty_directories_from_cache as tree command displays empty directories
  • doing the above fixed another test case
  • change type of CLI parameter for -P from Option<String> to Vec<String> to be in accordance with tree command
  • fix expected result test test_filter_txt_files

TODO:
Test test_filter_txt_files still breaks. The expected value is based on tree's output, therefore it's correct. The logic in this crate needs to be validated and corrected.

- also add test case
- remove `fn remove_empty_directories_from_cache` as `tree` command
  displays empty directories
- doing the above fixed another test case
- change type of CLI parameter for `-P` from `Option<String>` to
  `Vec<String>` to be in accordance with `tree` command
- fix expected result test `test_filter_txt_files`

TODO:
Test `test_filter_txt_files` still breaks. The `expected` value is based
on `tree`'s output, therefore it's correct. The logic in this crate
needs to be validated and corrected
@ByteBaker
Copy link
Contributor Author

@sighol This one is not final by any means. I need your help/advice here.

I took liberty in deleting fn remove_empty_directories_from_cache because tree does not exclude empty directories and this was causing a test to fail where the folder tests/simple/yyy/k was being omitted in the output.

One of the tests still breaks. The expected value is correct and based on tree's output. I need to check why the other directories aren't being listed after a match is found for the glob when running -P "*.txt".

Your advice would be much helpful. You can merge this one should you deem fit, but I'd personally prefer to fix the test before we do so.

@ByteBaker
Copy link
Contributor Author

ByteBaker commented Sep 8, 2024

I ended up debugging this and turns out we had a lot of redundant code convoluting the logic inside FilteredIterator. We didn't need the fields cache, skip, or next_item.

  • skip and cache were redundant because a cache is already present inside FileIterator. We were just taking the files from FileIterator::cache and putting it inside FilteredIterator::cache (and sometimes inside next_item too). All of this is gone now.
  • skip was a flag determined in the beginning if any filters were present or not. These are being handled inside FileIterator and the flag made no sense so I removed it too. As a result fn FilteredIterator::skip_filter is also gone.

All test cases now pass, output always matches that of tree command. Lemme know what you think. If everything looks good, please go ahead and merge it.

@sighol sighol merged commit 813546a into sighol:master Sep 9, 2024
3 checks passed
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