BF: strip away any prefix before ":" while determining numeric dandiset identifier#1751
BF: strip away any prefix before ":" while determining numeric dandiset identifier#1751candleindark merged 2 commits intomasterfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1751 +/- ##
==========================================
- Coverage 75.05% 75.05% -0.01%
==========================================
Files 84 84
Lines 11874 11873 -1
==========================================
- Hits 8912 8911 -1
Misses 2962 2962
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes identifier extraction to handle prefixed dandiset identifiers (e.g., "DANDI-SANDBOX:217838") by generalizing the prefix-stripping logic to remove any prefix before the last colon, rather than only stripping the "DANDI:" prefix.
Key Changes:
- Replaced
startswith("DANDI:")check withsplit(":")[-1]to extract the numeric identifier from any prefixed format - Updated comments to reflect the uncertainty about the original PR #348 and mark the code for future refactoring
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@yarikoptic To get the CI tests to pass, we need to first merge #1744. |
…et identifier We have it confusing, as the identifier in the client model is just the numeric part. We should have called it differently from the metadata model identifier
yarikoptic
force-pushed
the
rf-DANDI-prefix
branch
from
faebfa1 to
96f056c
Compare
|
merged, and marked mystery resolved as IIRC we figured it out on Friday as it was not the identifier within dandiset.yaml as was retrieved from non-vendored back then instance. |
candleindark
left a comment
candleindark
left a comment
There was a problem hiding this comment.
Add a small change. Otherwise, it is approved.
Co-authored-by: Isaac To <candleindark@users.noreply.github.com>
|
@candleindark you would need to reapprove for it to become mergeable... feel welcome to click merge as well, not yet sure if worth releasing right away, may be more would come up? |
github-project-automation
bot
moved this from In Progress
to Done
in Multi-DANDI instance support
|
🚀 PR was released in |
We have it confusing, as the identifier in the client model is just the numeric part. We should have called it differently from the metadata model identifier.
That original code which added those divergences though seemed tried to make meditor possible:
Without this fix, we get confusing
which accents such divergence of definition of "identifier". If we just remove stripping away the prefix, then upload stalls while retrying and getting 500s from the server (nothing in dandi-cli logs was useful to point to the issue).
TODOs: