-
Notifications
You must be signed in to change notification settings - Fork 11
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: update models to do more validation checks #42
Conversation
Codecov Report
@@ Coverage Diff @@
## master #42 +/- ##
==========================================
+ Coverage 95.72% 95.78% +0.06%
==========================================
Files 11 11
Lines 1005 1021 +16
==========================================
+ Hits 962 978 +16
Misses 43 43
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
related to: dandi/dandi-archive#349 |
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.
primary comment is -- if pydantic already match
es (and not searches) for regex
-- no need to anchor any of the regexes
"[a-f0-9]{4}[-]*[a-f0-9]{4}[-]*[a-f0-9]{12}$" | ||
) | ||
DANDI_DOI_PATTERN = r"^10.80507/dandi\.\d{6}/\d+\.\d+\.\d+" | ||
DANDI_PUBID_PATTERN = r"^DANDI:\d{6}/\d+\.\d+\.\d+" |
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.
same here -- .match
instead of /.search
and remove ^
here?
@@ -71,7 +71,7 @@ def _basic_publishmeta(dandi_id, version="v.0", prefix="10.80507"): | |||
"schemaKey": "PublishActivity", | |||
}, | |||
"version": version, | |||
"doi": f"{prefix}/dandi.{dandi_id}.{version}", | |||
"doi": f"{prefix}/dandi.{dandi_id}/{version}", |
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.
not sure if having that extra /
(although allowed AFAIK) wouldn't confuse some system. Prior discussion etc dandi/dandi-archive#175 (comment)
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.
i think /
is cleaner and still allowed, since our tests pass with datacite, but no strong opinion on my side other than it's a nice semantic break.
we just have to decide and then make sure @dchiquito uses that.
Co-authored-by: Yaroslav Halchenko <[email protected]>
adding release tag. @dchiquito - any comments on this PR? otherwise will merge. |
This improves a bunch of validation specs for publish. However, the current implementation is completely predicated on production servers and prefixes.
Calling this a draft for now while we think about how to get info about the domain in which the schema is running. technically for most users everything should validate against the production sites. but devs may need other things.