Skip to content

fix: traverse directories to allow pattern matching of files within them #259

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

Merged
merged 6 commits into from
Jun 13, 2025

Conversation

a3ng7n
Copy link
Contributor

@a3ng7n a3ng7n commented Apr 18, 2025

Allows pattern matching to check against files within directories. Changes the logic of _should_include to traverse all directories such that pattern matching can happen to files within them. Also adds some logic to _process_node to not include folders which have no children; i.e. when no files within them had passed the include/ignore filter.

Also adds a few unit tests of some include/ignore patterns against the sample temp_directory.

imo a more extensible solution would be to use glob, but that seemed like it'd require a more significant restructuring.

closes #224

@cyclotruc
Copy link
Member

@a3ng7n Thank you very much for your contribution
This seems like a reasonable change, some tests seem to fail but that's probably easy to fix
I would have to do some more testing before merging but looks good overall 👍

@a3ng7n
Copy link
Contributor Author

a3ng7n commented Apr 18, 2025

@a3ng7n Thank you very much for your contribution This seems like a reasonable change, some tests seem to fail but that's probably easy to fix I would have to do some more testing before merging but looks good overall 👍

awesome! I just saw that test failing; there was a dictionary access inside a formatted string - since the pre-commit formatter kept reverting the switch to "".format(...) I just switched it to use regex instead.

@a3ng7n
Copy link
Contributor Author

a3ng7n commented Apr 18, 2025

Oops looks like I left some of the newer typing syntax in - I'll take care of it in the morning.

@a3ng7n
Copy link
Contributor Author

a3ng7n commented Apr 18, 2025

I'm pretty sure it'll work now - tested locally on 3.8 and 3.12; hoping the versions in-between 'just work'

@a3ng7n
Copy link
Contributor Author

a3ng7n commented Apr 23, 2025

just wanted to bump this - it should pass the ci unit tests now

@a3ng7n
Copy link
Contributor Author

a3ng7n commented May 2, 2025

just pinging again - let me know if you'd like any help testing

@r6hk
Copy link

r6hk commented May 3, 2025

Thank you for your contribution. I really hope this PR gets approved.😭😭

@thegreyd
Copy link

thegreyd commented May 13, 2025

Hey @cyclotruc , hoping you can review and merge, because main is basically broken right now..

@hnykda
Copy link

hnykda commented May 15, 2025

I agree, I cannot really use the tool at all at the moment as I cannot figure out how to list files from subdirectory

@RonanKMcGovern
Copy link

Hi folks, any reason this isn't merged? I tested it and it works well.

@hnykda
Copy link

hnykda commented Jun 11, 2025

repo is abandoned?

@filipchristiansen
Copy link
Contributor

Hi all — @hnykda, @RonanKMcGovern, @r6hk, and especially @a3ng7n,

First, sincere apologies for the long silence. I (and @cyclotruc) let this PR sit far too long. Between a couple-week holiday, my PhD crunch, and @cyclotruc focusing on other work (see http://pad.ws), we simply failed to keep up. Explanation, not excuse — we’ll communicate better from here on.

@filipchristiansen filipchristiansen self-requested a review June 13, 2025 15:23
Copy link
Contributor

@filipchristiansen filipchristiansen left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@filipchristiansen filipchristiansen merged commit 789be9b into coderamp-labs:main Jun 13, 2025
31 of 36 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.

bug: include pattern matching not working
8 participants