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

reformat and added sh:info's #366

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

Conversation

coret
Copy link
Contributor

@coret coret commented Dec 5, 2021

Note: tests have not been adjested for sh:info's

@ddeboer ddeboer self-assigned this Dec 7, 2021
@@ -19,252 +19,641 @@
reg:DatacatalogShape
a sh:NodeShape ;
sh:targetClass schema:DataCatalog ;
rdfs:label "Datacatalogus"@nl, "Data catalog"@en ;
rdfs:comment "Een datacatalogus bestaat uit een set van datasetbeschrijvingen"@nl, "A data catalog consists of a set of dataset descriptions"@en ;
sh:name "Datacatalogus"@nl,
Copy link
Member

@ddeboer ddeboer Dec 17, 2021

Choose a reason for hiding this comment

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

Why did you change rdfs:label to sh:name? This is not documented in the PR’s description. Examples in the SHACL specs use rdfs:label too.

# Required properties
reg:SchemaDatasetProperty,
#
# Required properties
Copy link
Member

@ddeboer ddeboer Dec 17, 2021

Choose a reason for hiding this comment

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

Tabs again. 😒

Just having one comment line looks fine. Why do we need three?

sh:path schema:license ;
sh:nodeKind sh:IRIOrLiteral ;
sh:minCount 1 ;
sh:message "Het wordt aangeraden om, indien aanwezig en bekend, een licentie op te nemen."@nl,
"It is recommended to include a license, if present and known."@en ;
sh:severity sh:Info ;.
Copy link
Member

Choose a reason for hiding this comment

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

Is ;. intended?

"It is recommended to include a license, if present and known."@en ;
sh:severity sh:Info ;.

reg:SchemaDatasetLicensePropertyMax
Copy link
Member

Choose a reason for hiding this comment

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

Why did you split this into two properties? Again, not explained in the PR.

Copy link
Member

@ddeboer ddeboer left a comment

Choose a reason for hiding this comment

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

In terms of maintainability, I really don’t like the duplicated constraints for min and max. They make violation messages only marginally clearer. I think we can achieve the same with a single constraint and improved wording, for example ’a dataset must include exactly one license’.

@ddeboer ddeboer mentioned this pull request Dec 17, 2021
@ddeboer
Copy link
Member

ddeboer commented Dec 17, 2021

For a minimal example of sh:Info with passing tests, see #387.

@ddeboer ddeboer assigned coret and unassigned ddeboer Jan 11, 2022
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.

None yet

2 participants