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

Final adjustments and corrections for schema validation #2049

Merged
merged 4 commits into from
Aug 12, 2024

Conversation

TobiasNx
Copy link
Contributor

@TobiasNx TobiasNx commented Aug 8, 2024

See #1340

This PR:

  • Adds missing id for "DDC-Sachgruppen der ZDB"
  • Adds a variation in the skosConcept schema to either reqiure id AND label or jus label or notation without id.
  • Fix a formal error in the json schema, instead of tuple validation, the items element has to be an object instead of an array, otherwise it would only check the first element.
  • Adds a missing value to the type schema.

@TobiasNx TobiasNx requested review from maipet and acka47 August 9, 2024 07:12
Copy link
Contributor

@acka47 acka47 left a comment

Choose a reason for hiding this comment

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

The change to skosConcept.json looks good but we have to discuss the id for DDC-Sachgruppen der ZDB.

@@ -296,8 +296,10 @@ do list(path:"084??", "var":"$i")
if any_match("$j","\\d{3}(\\.\\d{1,3})?|[BKS]")
if any_equal("$i.q","DE-600")
add_field("subject[].$last.source.label","DDC-Sachgruppen der ZDB")
add_field("subject[].$last.source.id","https://zeitschriftendatenbank.de/fileadmin/user_upload/ZDB/pdf/zdbformat/5080.pdf")
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting choice of id for this source. While I like the pragmatism, this sets a new precedent linking to the description of a controlled vocabulary in a documentation PDF. As I have no better idea, I guess we can leave it for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

On the other hand, we should probably better leave out the id field and add the documentation link in another field, e.g. seeAlso.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I already had a bad feeling a while ago about id: #1848 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But we decided off board to go on for a lack of a better id.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, then let's go. +1

@acka47 acka47 self-requested a review August 9, 2024 11:43
@TobiasNx TobiasNx merged commit bb934e6 into master Aug 12, 2024
1 check passed
@TobiasNx TobiasNx deleted the 1340-finalAdjustmentsAndCorrections branch August 12, 2024 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants