Remove hardcoded vendor specific information#1760
Conversation
23ebb10 to
6a46ebf
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1760 +/- ##
==========================================
+ Coverage 75.03% 75.07% +0.04%
==========================================
Files 84 84
Lines 11889 11910 +21
==========================================
+ Hits 8921 8942 +21
Misses 2968 2968
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:
|
6a46ebf to
1536238
Compare
|
@yarikoptic Is Lines 131 to 139 in 7cf6ace identifier of the Dandiset model ever be a dictionary?
|
running tests did not step that way and originally added in 9006f96 which lead to dandi/dandi-archive#63 (comment) which shows that indeed it something which was changed on dandi-archive side awhile back. Thus we can safely remove this block IMHO. |
So that the checks of `id` and `identifier` in metadata allow different instance names
So that the check of `id` in metadata allows different instance names
1536238 to
c28e19d
Compare
|
should it be taken out of the draft? |
It is a base PR that I will merge other PRs into. I will seek approval from you to merge other PRs into this one. Each of the other PRs warrant a judgment. An example of this those PRs is #1767. |
To reflect that instance names are no longer restricted to "DANDI"
|
I think we can leave the "DANDI:" string in the following lines in place. It is specific to how datasets in the DANDI Archive are published on identifiers.org. dandi-cli/dandi/dandiarchive.py Lines 608 to 617 in f5b8bc3 @yarikoptic Let me know if I am wrong. |
…from_doi` This is only a replacement for the code that strips the "DANDI:" prefix. The replacement works for dandisets of different DANDI instances. However, `update_dandiset_from_doi` is far from robust. This replacement doesn't fix underlying weakness stems from assumptions. Further improvements of `update_dandiset_from_doi` are needed.
|
I think that keys such as For example, Lines 317 to 324 in f5b8bc3 @yarikoptic Let me know if I am wrong. |
There was a problem hiding this comment.
Pull request overview
This PR removes hardcoded vendor-specific information (particularly "DANDI:" prefix references) from the codebase and replaces them with vendor-agnostic implementations using configuration from dandischema. The changes allow the CLI to work with different DANDI instances without hardcoded dependencies on the DANDI Archive instance.
Key changes:
- Replaced hardcoded "DANDI:" string checks with dynamic pattern matching using
ID_PATTERNfrom dandischema - Removed vendor-specific validation logic from identifier parsing
- Updated prefix stripping to handle any known instance name instead of just "DANDI:"
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| dandi/tests/test_download.py | Updated assertion to use pattern matching instead of hardcoded "DANDI:" prefix |
| dandi/dandiset.py | Removed hardcoded validation for "DANDI" propertyID in identifier records |
| dandi/dandiarchive.py | Updated comment to be vendor-agnostic |
| dandi/cli/tests/test_service_scripts.py | Changed assertions to use pattern matching with ID_PATTERN |
| dandi/cli/cmd_service_scripts.py | Generalized prefix stripping to handle any known instance name |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
* feat: reimplement `dandi.utils.is_url` So that it can determine whether a given string is a standard HTTP, HTTPS, FTP URL, or DANDI URL more precisely. Additionally, this solution supports DANDI URL of different DANDI archive instances. * test: Add tests for `dandi.utils.is_url`
|
Yes, both identifiers and checksum are ok to remain dandi. Note that some test run is failing... Otherwise this is ready? |
Yes, this is ready. The failure in test is unrelated this PR since it appears in #1768 as well. I am looking into the cause of the failure now. |
Configuer CI to test against a vendor specific DANDI API other than the default instance of DANDI API
|
🚀 PR was released in |
This PR closes #1752. It removes any hardcoded vendor specific information that are now made available through a
Configobject defined in dandischema. Additionally, it includes the changes in GH workflow in #1771 to verify the correctness of the removal of the vendor specific information.Specifically, this PR replaces any use of the
"DANDI:"string that is associated with the DANDI Archive instance with vendor agnostic (or DANDI instance agnostic) setup.TODOs:
dandi.utils.is_url#1767 into this PR.Notes:
dandi service-scripts publish-dandiset-version-doiis not replaced/removed in this PR. It will be part of Make thedandi service-scripts publish-dandiset-version-doicommand DANDI instance specific, incorporating vendor information of a particular DANDI instance #1704 instead.obolibrary#1769.Release Notes
Removed hardcoded vendor specific info so that dandi-cli can now connect to different DANDI instances with different vendor specific info.