-
Notifications
You must be signed in to change notification settings - Fork 24
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
corrected session_id by replacing invalid characters with underscores… #1227
base: main
Are you sure you want to change the base?
Conversation
… in automatic_dandi_upload
for more information, see https://pre-commit.ci
Thanks for the draft @anoushkajain. Don't forget to add a test for it and ping me when you think is ready or you need any help with it. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1227 +/- ##
==========================================
- Coverage 89.92% 89.91% -0.02%
==========================================
Files 131 131
Lines 8411 8412 +1
==========================================
Hits 7564 7564
- Misses 847 848 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
What do you think of creating more than one file given that dandi organize is meant to work across a set of many files. Let's do 4-5-6 or something like that.
Also, for the last bit of the test I think it is better to just be explicit instead of relying on a loop to make the error obvious and transparent.
What do you think? @alessandratrapani @anoushkajain
solves issue #1156