inveniosoftware / datacite

Python API wrapper for the DataCite API.
https://datacite.readthedocs.io
Other
27 stars 33 forks source link

Fixed creators affiliations #74

Closed thorge closed 1 year ago

thorge commented 2 years ago

Description

Fixed typo in affiliations property name and added affiliationIdentifierScheme to required fields. Also removed sub-property schemeUri, that appears to be a mandatory field (see metadata kernel / documentation, p. 15) but is not contained in examples or real data API responses (example response). Also the examples don't validate against the schema, since they don't include a scheme URI.

If schemeUri should be added again, just let me know and I will update the PR. If not, the metadata kernel should be updated accordingly.

Btw. there is no 4.4 schema version for now and I did not check the older versions for same problems, since they are outdated anyway.

Checklist

Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:

tmorrell commented 2 years ago

Thanks for your contribution! I'll start by noting that this is not a DataCite-managed project, so we don't have any particular ability to update their API or documentation.

affiliations is only the internal definition label in the schema, and won't affect schema validation or output. That being said I agree it's confusing and should be changed.

schemaUri is optional property 7.5.c in schema 4.3. The example will validate without it, since it is not required.

There are some issues with how affiliationIdentifier was implemented by DataCite that make it confusing. The DataCite documentation is not the clearest, but affiliationIdentifierScheme is only listed as required if an affiliationIdentifier is present. It's not correct to make this required for every affiliation. However, if you look at the official XML schema both affiliationIdentifier and affiliationIdentifierScheme are actually labeled as optional https://schema.datacite.org/meta/kernel-4.3/metadata.xsd. I'm not sure if it is even possible to implement this logic described in the documentation in a jsonschema or the xml version, because affiliationIdentifier isn't an object.

I'd be happy to merge in the change from affiliations to affiliation in the description label. Just use a commit message like "schema: update affiliation definition label"

thorge commented 2 years ago

Thank you very much for the feedback. I actually overlooked the fact that this is not the official repo, but I may be able to contribute anyway.

That affiliation is only the internal definition name in the schema and has no influence on schema validation or output is correct.

schemaUri seems to be an optional property (at least the DataCite API does not validate against it), but the documentation 7.5.c in schema 4.3 says that the property should occur exactly 1 time. I agree with you that this property only makes sense if nameIdentifierScheme has a value and that property has a cardinality of 0-1.

It is also true that the official XML schema has both affiliationIdentifier and affiliationIdentifierScheme marked as optional. It may indeed be true that implementing this logic described in the documentation in a jsonschema or the xml version is not possible. But in 2.5.b it says again that the property should occur exactly 1 time, which is a bit confusing. But I guess it's analog to the example above. This property only makes sense if affiliationIdentifier has a value and that property has a cardinality of 0-1.

How about the renaming of affiliations. It's not clear to me, wheter I should create a new MR for the change or modify my commits accordingly?

tmorrell commented 2 years ago

No question that the documentation is confusing, but until we hear differently from DataCite I think the current implementation is correct.

You can either modify this PR for the affiliations change or send in a new one. Whatever is easier for you.

thorge commented 2 years ago

Please check if my changes are okay. I would suggest squashing the commits in merge, otherwise it could look unattractive in the history ;)

tmorrell commented 2 years ago

Looks good, but I need to do some updates to the CI so the tests pass. Nothing to do with your changes, but it might take me a little bit to get to it.

fenekku commented 1 year ago

Coordination:

Looks like this was pending a review by @kpsherva for a couple of weeks. The changes were simple enough for me to approve. Otherwise I would have asked for someone Karolina to take another look/recommend someone else.

This is ready for merge @tmorrell .