Skip to content
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

Schema 4.6 #889

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Schema 4.6 #889

wants to merge 11 commits into from

Conversation

kaysiz
Copy link
Contributor

@kaysiz kaysiz commented Oct 8, 2024

Purpose

Add new values for various attributes

Spec documents:

closes: #887 #888 #886 #884 #883

Approach

Open Questions and Pre-Merge TODOs

Learning

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

  • New feature (non-breaking change which adds functionality)

  • Breaking change (fix or feature that would cause existing functionality to change)

Reviewer, please remember our guidelines:

  • Be humble in the language and feedback you give, ask don't tell.
  • Consider using positive language as opposed to neutral when offering feedback. This is to avoid the negative bias that can occur with neutral language appearing negative.
  • Offer suggestions on how to improve code e.g. simplification or expanding clarity.
  • Ensure you give reasons for the changes you are proposing.

@kaysiz kaysiz self-assigned this Oct 8, 2024
@kaysiz kaysiz marked this pull request as ready for review October 14, 2024 06:49
@kaysiz kaysiz added the create preview app Create a review app on Vercel label Oct 23, 2024
@kaysiz kaysiz temporarily deployed to vercel-bracco-preview October 23, 2024 13:46 — with GitHub Actions Inactive
Copy link

Deploy preview for bracco ready!

✅ Preview
https://bracco-rdckjb3ze-datacite.vercel.app

Built with commit 9bd8689.
This pull request is being automatically deployed with vercel-action

@github-actions github-actions bot removed the create preview app Create a review app on Vercel label Oct 23, 2024
Copy link
Contributor

@svogt0511 svogt0511 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaysiz - A couple of comments in the code. Also I left some comments at the vercel preview at https://bracco-rdckjb3ze-datacite.vercel.app/dois/10.82531%2Fta9w-p411. Let me know if you don't see those comments.

Copy link
Contributor

@svogt0511 svogt0511 Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a comment. I never realized that we tracked flavors of contributor types. Are these defined somewhere?
For example, where is it defined that translator is not an organizationalContributor. (Just curious)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case however Transalator can be both personal or organization. From the pattern it should probably just stay in the contributorTypes list. @codycooperross @KellyStathis can weigh in on this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense to me that a Translator could be either Organizational or Personal.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a question. Is there a reason why we use both schemaLocation for kernel-4 and kernel-4.6? I thought that using kernel-4 would be the latest.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@svogt0511 the first part is the namespace and the second is the url to the file that will validate the data

@@ -43,13 +43,16 @@ const relationTypeList = [
'Is original form of',
'Is identical to',
'Is collected by',
'Collects'
'Collects',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codycooperross @KellyStathis What order should we follow for this list?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the past, I think this list has been additive, i.e. we just keep adding new values at the bottom :) What are some other options? Alphabetical, according to the list in the schema docs: https://datacite-metadata-schema.readthedocs.io/en/4.5/appendices/appendix-1/relationType/ —any others?

this.fragment.set('relatedIdentifier', value);
this.fragment.set('relatedIdentifierType', 'RRID');
this.set('controlledIdentifierType', true);
break;
Copy link
Contributor

@codycooperross codycooperross Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good for RRID, and there shouldn't be any collisions with other IDs :) Just curious—did you determine that CSTR might have collisions?

Copy link
Contributor

@svogt0511 svogt0511 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@codycooperross
Copy link
Contributor

@kaysiz I'm having some trouble entering the CSTR relatedIdentifierType in the form. It seems like no matter what form the relatedIdentifier is, I get the following error at the bottom of the form:

Screen Shot 2024-11-07 at 3 55 20 PM Screen Shot 2024-11-07 at 3 55 29 PM

I might be missing something, but can you take a look? The CSTR example I used was 31253.11.sciencedb.00024, found here: https://www.scidb.cn/en/detail?dataSetId=829484098497019904

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment