-
Notifications
You must be signed in to change notification settings - Fork 23
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
feat(doi): core support for DOI creation against DataCite API #172
Conversation
The style in this PR agrees with This formatting comment was generated automatically by a script in uc-cdis/wool. |
… updates for discovery metadata
gen3/doi.py
Outdated
} | ||
|
||
endpoint = self.api.rstrip("/") + f"/dois/{identifier}" | ||
logging.info(f"PUT-ing to {endpoint}...") |
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 this is supposed to be DELETE instead of PUT?
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.
yes, good catch
f"Consider providing this. Default will be used: {DataCite.DEFAULT_DOI_CONTACT_INFORMATION}" | ||
) | ||
metadata[prefix + "contact"] = DataCite.DEFAULT_DOI_CONTACT_INFORMATION | ||
if additional_metadata.get("contact"): |
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.
This is for my own understanding. Why are we removing some of these fields from addiotional_metadata
?
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.
they're getting adding to the metadata and then removed from the additional so that when we do this:
metadata.update(additional_metadata_with_prefix)
we don't get duplicates. Basically, this is parsing known fields, adding them to metadata, removing them from the additional_metadata so that only unknown fields gets added and there aren't duplicates in the final data.
NOTE: If you use anything other than the schema this was built for (v4.4), | ||
this logic will need to be updated. | ||
|
||
From the DataCite Metadata Schema V 4.4: |
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.
How do we want to handle schema change in the future? Can both an old schema and a new schema be valid?
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.
The schema can dramatically change the fields and options, so if we ever update I think we'll need to rewrite a lot of this. Existing records with still have the existing metadata schema, but new stuff would be the new schema. I don't think we should keep around multiple versions for too long though (so we'd likely want to migrate old schema records if we ever update). Gen3 should be opinionated on which version to use so I would expect the SDK to be updated to whatever version we want to promote (which maybe some conversation / migration utilities in the future)
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.
lgtm!
New Features
Breaking Changes
Bug Fixes
Improvements
Dependency updates
Deployment changes