-
Notifications
You must be signed in to change notification settings - Fork 12
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
use UUIDs for default file ID manager implementations #30
Conversation
Codecov ReportBase: 79.39% // Head: 79.68% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #30 +/- ##
==========================================
+ Coverage 79.39% 79.68% +0.28%
==========================================
Files 5 5
Lines 495 502 +7
Branches 68 68
==========================================
+ Hits 393 400 +7
Misses 80 80
Partials 22 22
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Sorry, I'm confused. I thought @davidbrochart's PR to allow arbitrary Contents Managers had defined an ABC class named |
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.
Hi @dlqqq - these changes look good relative to the conversion to UUID. Most of my comments are a carryover from the late review on #24 and assumptions made about what arbitrary fileId managers might need. I apologize for those being a bit off-topic.
Since it's clear that those are potential changes in a separate PR, I'm approving these changes relative to the conversion to UUID. We can follow up regarding my comments here or not.
Thanks for the ping.
@kevin-bates Thank you for your comments! I'm just going to go ahead and merge this for now since none of them pertain to the UUID implementation. We can keep discussing in this PR and #24 of course. |
@kevin-bates Actually, let's just keep all discussion local to #24 for cleanliness. I'll copy my responses over there. |
Closes #3.