-
Notifications
You must be signed in to change notification settings - Fork 98
BUG: Fix bug with path search #1379
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
Conversation
| if datatype is not None: | ||
| datatype = _ensure_tuple(datatype) | ||
| search_str = f"**/{'|'.join(datatype)}/.*" | ||
| search_str = f"**/{'|'.join(datatype)}/*.*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the real bug. All else is just cleanups + test
|
Yikes the benchmark test takes 60 seconds (!). It also no longer passes for me locally. I'll see if the speedup comes from in which case it should be used in both code paths. But that would be weird... |
|
I'd also like to fixup a couple things about #1355 while this is open. I see you already restored the
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1379 +/- ##
==========================================
+ Coverage 97.37% 97.47% +0.09%
==========================================
Files 40 40
Lines 8966 9024 +58
==========================================
+ Hits 8731 8796 +65
+ Misses 235 228 -7 ☔ View full report in Codecov by Sentry. |
|
Okay should be good to go @drammock |
drammock
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just one tiny docstring tweak. I'll apply it and merge. Thanks @larsoner
Closes #1376