-
Notifications
You must be signed in to change notification settings - Fork 23
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
Fix path expander II #679
Fix path expander II #679
Conversation
@h-mayorquin thanks for working on this. Let me see if it's possible with regex |
oh, that's not easy with with parse library. hmm. |
@h-mayorquin what do you think of this? I factored out the file creation logic so it can be used my multiple tests and I added a test case to ensure that the parser is not combining multiple nested directories into a single metadata field |
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.
@bendichter This is OK to me. The code duplication across the two tests did bother me and I was thinking to use a module scoped pytest fixture to get rid of it. I was on the edge though because of the trade-off between your test being very explicit which is good and some code duplication which is bad.
But if you think this makes more sense and I was already on the edge it seems tha the average favors a bit of abstraction.
If you are Ok with this we should mege!
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #679 +/- ##
=======================================
Coverage 92.06% 92.06%
=======================================
Files 115 115
Lines 5960 5963 +3
=======================================
+ Hits 5487 5490 +3
Misses 473 473
Flags with carried forward coverage won't be shown. Click here to find out more.
|
After #675 @bendichter mentioned that we are still missing the case of nested over matching:
and proposed a solution of excluding metadata entries that have file separators (slashes in posix or double front slashes on windows) from the matches. This PR implements this solution and amends the tests to reflect that.
This should fix #674