-
Notifications
You must be signed in to change notification settings - Fork 10
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
ENH: adopt sanitize_value from dandi-cli and use for sanitization of identifier #175
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #175 +/- ##
==========================================
+ Coverage 97.74% 97.75% +0.01%
==========================================
Files 16 16
Lines 1727 1739 +12
==========================================
+ Hits 1688 1700 +12
Misses 39 39
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
you could use an asset in the tests directory and change the "path" key and subject identifier keys to make the aggregate function raise that assertion you put. |
I didn't add assertion in this PR. In this PR such assertion would not be appropriate since code would still allow for multiple subjects from a single path. |
you could manually assert in the test. |
well, all tests assert "manually", the question what and in which test.... well -- since confirmed empirically, just feel free to either merge as is, or extend with the test as you see fit. |
@yarikoptic - this needs to be updated and merged for #172 |
This seems to Closes #172 as well
e8aba8e
to
61c374d
Compare
Closes #172 as empirically shown
@satra do you see a quick way to add a test?
TODO