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

Update the formal vocabulary files #920

Merged
merged 12 commits into from
Sep 3, 2022
Merged

Update the formal vocabulary files #920

merged 12 commits into from
Sep 3, 2022

Conversation

iherman
Copy link
Member

@iherman iherman commented Aug 25, 2022

This PR creates a v2 version of the RDFS vocabulary definition of the data model. Eventually, just as the context v2 will supersede the current v1, this vocabulary definition will supersede the current one.

The plan is to catch up with the evolution of v2 context and keep the two in sync.

Because there is no diff (and there were lots of small changes that would make a diff unusable anyway, here are the main changes on the file(s):

Editorial changes:

  • updated to latest respec
  • updated the title of the document, referring to v2.0
  • changed the list of editors
  • minor CSS addition to ensure a proper formatting of terms
  • removed the generation of the context file (it is maintained elsewhere, having this file here is probably a leftover which only create confusion; we should not have a context at two different places)
  • added an explicit version info into the html template (owl:versionInfo is not o.k., it would have to take an IRI as an object)
  • removed terms that are not specified in this vocabulary from the listing of term definitions in §4
  • added (in the HTML version) an id attribute to all terms, so that something like .../#evidence would properly dereference.

Handling bugs in the previous version:

  • added the odrl namespace (which is used) to the list of used namespaces in the HTML document , and removed sec (which is not used)
  • added the VerifiableCredentialGraph class and changed the domain of the verifiableCredential property, to fix Bug in the credential vocabulary specification #770
  • modified mk_vocab.rb not to add an erroneous dc:imports triple that had no object

Note that this PR does not yet sync up with the latest version of the context file, as merged in #905. That should come in a new PR if and when this one is accepted and merged.

To make the review easier, here are the previews of the three generated file:

Fix #916

@iherman iherman requested a review from msporny as a code owner August 25, 2022 04:30
@iherman iherman requested review from dlongley and pchampin August 25, 2022 04:31
Copy link
Contributor

@pchampin pchampin left a comment

Choose a reason for hiding this comment

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

The way the documentations of terms are written is ambiguous. Some are focused on the conceptual level (e.g. "The value of the credentialSchema property MUST be one or more data schemas") while other are focused on the JSON-LD syntactic level (e.g. "The value of the credentialStatus property MUST include the id property, which MUST be a URL").

This should be harmonized, all focusing on the conceptual level, if indeed the JSON-LD context is maintained elsewhere.

In the same line, the Term Definitions section in the HTML document seems to be mostly focused on JSON-LD, not RDFS, so I would simply remove it.

vocab/credentials/v2/credentials.ttl Outdated Show resolved Hide resolved
vocab/credentials/v2/credentials.ttl Outdated Show resolved Hide resolved
vocab/credentials/v2/mk_vocab.rb Outdated Show resolved Hide resolved
vocab/credentials/v2/mk_vocab.rb Show resolved Hide resolved
vocab/credentials/v2/vocab.csv Outdated Show resolved Hide resolved
@iherman
Copy link
Member Author

iherman commented Aug 25, 2022

The way the documentations of terms are written is ambiguous. Some are focused on the conceptual level (e.g. "The value of the credentialSchema property MUST be one or more data schemas") while other are focused on the JSON-LD syntactic level (e.g. "The value of the credentialStatus property MUST include the id property, which MUST be a URL").

This should be harmonized, all focusing on the conceptual level, if indeed the JSON-LD context is maintained elsewhere.

I would agree with this. I note that the definitions are usually come as a copy of the data model spec. I still have to familiarize myself more with the model before I could provide a set of alternative texts, and the spec may also change, so I would keep that on a todo list and do that at some point. Again, point well taken.

In the same line, the Term Definitions section in the HTML document seems to be mostly focused on JSON-LD, not RDFS, so I would simply remove it.

Yes, I was wondering about that myself. This section does not bring anything new and valuable to the reader, and I was tempted to remove it altogether, but I shied away from it for now. @msporny is it o.k. to remove it?

@iherman
Copy link
Member Author

iherman commented Aug 25, 2022

@pchampin just to be on the safe side: is the text used for verifiableCredential and VerifiableCredentialGraph ok? These is the term that I have introduced to handle #770. I also know that the exact relationship of the URL for a graph and the graph itself is not 100% specified; I want to avoid unnecessary raised eyebrows...

@pchampin
Copy link
Contributor

@pchampin just to be on the safe side: is the text used for verifiableCredential and VerifiableCredentialGraph ok? These is the term that I have introduced to handle #770.

They look good to me, but in the class description, I would replace "Instances of this class identify RDF graphs" with "Instances of this class are RDF graphs".

I also know that the exact relationship of the URL for a graph and the graph itself is not 100% specified; I want to avoid unnecessary raised eyebrows...

The change above keeps us on the safe side, I think. It does not imply any means by which the object of the verifiableCredential property is linked to the "content" of the graph.

@iherman
Copy link
Member Author

iherman commented Aug 25, 2022

@pchampin just to be on the safe side: is the text used for verifiableCredential and VerifiableCredentialGraph ok? These is the term that I have introduced to handle #770.

They look good to me, but in the class description, I would replace "Instances of this class identify RDF graphs" with "Instances of this class are RDF graphs".

Well... a class is a collection of resources, more exactly URI-s to resources, right? Whether these URI-s are graphs or whether they identify graphs... we can get into a long discussion, see also https://www.w3.org/TR/rdf11-datasets/. That is why I tried to be as neutral as possible.

I also know that the exact relationship of the URL for a graph and the graph itself is not 100% specified; I want to avoid unnecessary raised eyebrows...

The change above keeps us on the safe side, I think. It does not imply any means by which the object of the verifiableCredential property is linked to the "content" of the graph.

@pchampin
Copy link
Contributor

Well... a class is a collection of resources, more exactly URI-s to resources, right?

No, I wouldn't put it that way.

Quoting [RDF11-MT]:

It is convenient to state the RDFS semantics in terms of a new semantic construct, a class, i.e. a resource which represents a set of things in the universe (...)

"A set of things in the universe" (a.k.a. resources), not a set of IRIs. The fact that those resources may be identified by IRIs is irrelevant to the class itself.

So, to develop my argument above:
a triple <#x> cred:verifiableCredential <#y> implies that

  • <#x> identifies an instance of cred:VerifiablePresentation
  • <#y> identifies an instance of cred:VerifiableCredentialGraph, which according to the definition is an RDF graph
    but the ontology itself is agnostic about how to find out what triples are contained in the graph identified by <#y>

To fill that gap, the common practice with VC is to put the graph identified by <#y> into a named graph whose graph name is <#y>. People might raise their eyebrow at that, but they can't blame the ontology for it -- which I think was your original question.

@iherman
Copy link
Member Author

iherman commented Aug 26, 2022

Well... a class is a collection of resources, more exactly URI-s to resources, right?

No, I wouldn't put it that way.

Quoting [RDF11-MT]:

It is convenient to state the RDFS semantics in terms of a new semantic construct, a class, i.e. a resource which represents a set of things in the universe (...)

"A set of things in the universe" (a.k.a. resources), not a set of IRIs. The fact that those resources may be identified by IRIs is irrelevant to the class itself.

Ok, I stand corrected...

So, to develop my argument above: a triple <#x> cred:verifiableCredential <#y> implies that

  • <#x> identifies an instance of cred:VerifiablePresentation
  • <#y> identifies an instance of cred:VerifiableCredentialGraph, which according to the definition is an RDF graph
    but the ontology itself is agnostic about how to find out what triples are contained in the graph identified by <#y>

Based on what you said above, I am fine with this formulation.

To fill that gap, the common practice with VC is to put the graph identified by <#y> into a named graph whose graph name is <#y>. People might raise their eyebrow at that, but they can't blame the ontology for it -- which I think was your original question.

Right. But I will not put this into the text itself:-) Just remaining terse.

@iherman
Copy link
Member Author

iherman commented Aug 26, 2022

@msporny @dlongley I have added a description (after having re-read the spec) for the credentialSchema, credentialStatus and credentialEvidence classes. Actually, I have restructured the descriptions a bit, putting the emphasis on the class descriptions (that characterize what the instances are for) and the properties only 'refer' to the class instances that they use. That seems to be cleaner to me.

@iherman
Copy link
Member Author

iherman commented Aug 26, 2022

@msporny @dlongley

(maybe that deserves a separate issue, you tell me.)

The specification makes a difference between a "credential" and a "verifiable credential", as well as a "presentation" and a "verifiable presentation", that could be roughly characterized by saying that a "verifiable credential" (resp. "verifiable presentation") is a temper evident "credential" (resp. "presentation"). If this distinction is important, then it would deserve a separation in the ontology, by defining a "Credential" class and a "Verifiable Credential" subclass. (resp. "Presentation" and "Verifiable Presentation"). This is easy to add to the ontology.

If we do so, the question is whether the domain of the various properties should be verifiable credentials or just credentials. It strikes that almost none of the properties in this vocabulary depend on the "verifiable" aspect, maybe except for "verifiableCredential" which, at least with its name, is bound to "verifiable". The only property that MUST have a domain of "verifiable ..." would be proof, but that is not defined in this vocabulary...

WDYT?

Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
@dlongley
Copy link
Contributor

dlongley commented Aug 29, 2022

@iherman,

The specification makes a difference between a "credential" and a "verifiable credential", as well as a "presentation" and a "verifiable presentation", that could be roughly characterized by saying that a "verifiable credential" (resp. "verifiable presentation") is a temper evident "credential" (resp. "presentation"). If this distinction is important, then it would deserve a separation in the ontology, by defining a "Credential" class and a "Verifiable Credential" subclass. (resp. "Presentation" and "Verifiable Presentation"). This is easy to add to the ontology.

IIRC, adding types in the ontology for this was avoided in the 1.0 work to keep down the complexity. I imagine we'll want to continue doing so as a trade off (simplicity over theoretrical purity), but we'll have to see. For this reason, I don't think we should add it in this PR.

@iherman
Copy link
Member Author

iherman commented Aug 30, 2022

I imagine we'll want to continue doing so as a trade off (simplicity over theoretrical purity), but we'll have to see. For this reason, I don't think we should add it in this PR.

Personally, I would still be in favour of having this (or, alternatively, the spec text should not make a difference between "Credential" and "Verifiable Credential") but I am o.k. postponing this discussion. If we close this PR I may raise an issue just to keep a trace for ourselves.

@@ -0,0 +1,382 @@
{
"@context": [
"https://w3id.org/security/v2",
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm... prefer to not do this... this will trigger an entire 2 other network requests, since v2 relies on v1... thats 3 network requests to load the complete v2 context...

Copy link
Contributor

@OR13 OR13 Aug 31, 2022

Choose a reason for hiding this comment

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

This can be avoided by simply copying the terms required by the vc data model... instead of copying everything...for example:

{
  "@context": [{
    "@version": 1.1
  }, "https://w3id.org/security/v1", {
    "AesKeyWrappingKey2019": "sec:AesKeyWrappingKey2019",
    "DeleteKeyOperation": "sec:DeleteKeyOperation",
    "DeriveSecretOperation": "sec:DeriveSecretOperation",
    "EcdsaSecp256k1Signature2019": "sec:EcdsaSecp256k1Signature2019",
    "EcdsaSecp256r1Signature2019": "sec:EcdsaSecp256r1Signature2019",
    "EcdsaSecp256k1VerificationKey2019": "sec:EcdsaSecp256k1VerificationKey2019",
    "EcdsaSecp256r1VerificationKey2019": "sec:EcdsaSecp256r1VerificationKey2019",
    "Ed25519Signature2018": "sec:Ed25519Signature2018",
    "Ed25519VerificationKey2018": "sec:Ed25519VerificationKey2018",
    "EquihashProof2018": "sec:EquihashProof2018",
    "ExportKeyOperation": "sec:ExportKeyOperation",
    "GenerateKeyOperation": "sec:GenerateKeyOperation",
    "KmsOperation": "sec:KmsOperation",
    "RevokeKeyOperation": "sec:RevokeKeyOperation",
    "RsaSignature2018": "sec:RsaSignature2018",
    "RsaVerificationKey2018": "sec:RsaVerificationKey2018",
    "Sha256HmacKey2019": "sec:Sha256HmacKey2019",
    "SignOperation": "sec:SignOperation",
    "UnwrapKeyOperation": "sec:UnwrapKeyOperation",
    "VerifyOperation": "sec:VerifyOperation",
    "WrapKeyOperation": "sec:WrapKeyOperation",
    "X25519KeyAgreementKey2019": "sec:X25519KeyAgreementKey2019",

    "allowedAction": "sec:allowedAction",
    "assertionMethod": {"@id": "sec:assertionMethod", "@type": "@id", "@container": "@set"},
    "authentication": {"@id": "sec:authenticationMethod", "@type": "@id", "@container": "@set"},
    "capability": {"@id": "sec:capability", "@type": "@id"},
    "capabilityAction": "sec:capabilityAction",
    "capabilityChain": {"@id": "sec:capabilityChain", "@type": "@id", "@container": "@list"},
    "capabilityDelegation": {"@id": "sec:capabilityDelegationMethod", "@type": "@id", "@container": "@set"},
    "capabilityInvocation": {"@id": "sec:capabilityInvocationMethod", "@type": "@id", "@container": "@set"},
    "caveat": {"@id": "sec:caveat", "@type": "@id", "@container": "@set"},
    "challenge": "sec:challenge",
    "ciphertext": "sec:ciphertext",
    "controller": {"@id": "sec:controller", "@type": "@id"},
    "delegator": {"@id": "sec:delegator", "@type": "@id"},
    "equihashParameterK": {"@id": "sec:equihashParameterK", "@type": "xsd:integer"},
    "equihashParameterN": {"@id": "sec:equihashParameterN", "@type": "xsd:integer"},
    "invocationTarget": {"@id": "sec:invocationTarget", "@type": "@id"},
    "invoker": {"@id": "sec:invoker", "@type": "@id"},
    "jws": "sec:jws",
    "keyAgreement": {"@id": "sec:keyAgreementMethod", "@type": "@id", "@container": "@set"},
    "kmsModule": {"@id": "sec:kmsModule"},
    "parentCapability": {"@id": "sec:parentCapability", "@type": "@id"},
    "plaintext": "sec:plaintext",
    "proof": {"@id": "sec:proof", "@type": "@id", "@container": "@graph"},
    "proofPurpose": {"@id": "sec:proofPurpose", "@type": "@vocab"},
    "proofValue": "sec:proofValue",
    "referenceId": "sec:referenceId",
    "unwrappedKey": "sec:unwrappedKey",
    "verificationMethod": {"@id": "sec:verificationMethod", "@type": "@id"},
    "verifyData": "sec:verifyData",
    "wrappedKey": "sec:wrappedKey"
  }]
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Which then loads v1:

{
  "@context": {
    "id": "@id",
    "type": "@type",

    "dc": "http://purl.org/dc/terms/",
    "sec": "https://w3id.org/security#",
    "xsd": "http://www.w3.org/2001/XMLSchema#",

    "EcdsaKoblitzSignature2016": "sec:EcdsaKoblitzSignature2016",
    "Ed25519Signature2018": "sec:Ed25519Signature2018",
    "EncryptedMessage": "sec:EncryptedMessage",
    "GraphSignature2012": "sec:GraphSignature2012",
    "LinkedDataSignature2015": "sec:LinkedDataSignature2015",
    "LinkedDataSignature2016": "sec:LinkedDataSignature2016",
    "CryptographicKey": "sec:Key",

    "authenticationTag": "sec:authenticationTag",
    "canonicalizationAlgorithm": "sec:canonicalizationAlgorithm",
    "cipherAlgorithm": "sec:cipherAlgorithm",
    "cipherData": "sec:cipherData",
    "cipherKey": "sec:cipherKey",
    "created": {"@id": "dc:created", "@type": "xsd:dateTime"},
    "creator": {"@id": "dc:creator", "@type": "@id"},
    "digestAlgorithm": "sec:digestAlgorithm",
    "digestValue": "sec:digestValue",
    "domain": "sec:domain",
    "encryptionKey": "sec:encryptionKey",
    "expiration": {"@id": "sec:expiration", "@type": "xsd:dateTime"},
    "expires": {"@id": "sec:expiration", "@type": "xsd:dateTime"},
    "initializationVector": "sec:initializationVector",
    "iterationCount": "sec:iterationCount",
    "nonce": "sec:nonce",
    "normalizationAlgorithm": "sec:normalizationAlgorithm",
    "owner": {"@id": "sec:owner", "@type": "@id"},
    "password": "sec:password",
    "privateKey": {"@id": "sec:privateKey", "@type": "@id"},
    "privateKeyPem": "sec:privateKeyPem",
    "publicKey": {"@id": "sec:publicKey", "@type": "@id"},
    "publicKeyBase58": "sec:publicKeyBase58",
    "publicKeyPem": "sec:publicKeyPem",
    "publicKeyWif": "sec:publicKeyWif",
    "publicKeyService": {"@id": "sec:publicKeyService", "@type": "@id"},
    "revoked": {"@id": "sec:revoked", "@type": "xsd:dateTime"},
    "salt": "sec:salt",
    "signature": "sec:signature",
    "signatureAlgorithm": "sec:signingAlgorithm",
    "signatureValue": "sec:signatureValue"
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

A better starting point would be:

{
 "@context": {
    "@version": 1.1,
    "@vocab": "https://w3id.org/security#"
  }
}

Since you can see that all the security concerns are actually mapped to w3id.org... not w3c.

Copy link
Member

@msporny msporny Aug 31, 2022

Choose a reason for hiding this comment

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

-1 for cascaded loading of security/v2 and security/v1 -- we want to get away from that in this iteration of the work.
-1 for use of @vocab for any JSON-LD context that touches security, we want to be specific about the terms we use, especially when those things are going to be digitally signed. Using @vocab causes the new "undefined terms detected" safety features of jsonld.js to not fire, and we definitely don't want to silence those safeties.

We should try to start from here since its much simpler (though, I'm fine w/ doing all of this in another PR):

https://github.com/w3c/vc-data-model/blob/main/contexts/credentials/v2

Copy link
Contributor

Choose a reason for hiding this comment

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

@msporny,

I think people are reacting to the fact that this PR re-defines the v2 JSON-LD Context...

Does it? I thought that was a totally different file that only the vocab used.

Re: throwing errors when using @vocab ...

Using @vocab is an explicit signal that the author does not want errors thrown for undefined terms, but instead wants them folded into some vocabulary without knowing what they are. So I don't expect safe mode to ever throw for those cases. Using @vocab is a sort of middle ground in the security space -- where unknown terms will be signed, but there will be no globally unambiguous definition for their meaning. The introduction of a new context could also "define them later" -- in ways that may contradict their previous (locally understood) meaning. This is the danger of using @vocab, but if that danger is understood, i.e., there is zero reliability with those terms at all, perhaps that's an acceptable compromise for allowing their use.

We should get a clear understanding for the potential advantage sought by using @vocab so it can be weighed transparently and appropriately against the disadvantages. Of course, if @vocab is only being used as a bad substitute for safe mode -- then it seems to me that safe mode is clearly better. I think there's more people are looking for though.

All that being said, the implementation of the new safe mode feature in jsonld.js (at least) actually uses a more comprehensive internal event system that is able to emit all kinds of events for debugging purposes (including ones around the use of @vocab). That feature hasn't been externalized yet with a clean API. But that feature will be a development / debugging tool as opposed to a data validation / protection feature like safe mode is.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think people are reacting to the fact that this PR re-defines the v2 JSON-LD Context in a way that 1) we don't want, and 2) is in conflict with what's already agreed to as a baseline by the group:

https://github.com/w3c/vc-data-model/pull/920/files#diff-0457f943f5c5375c7aa4592b374df978608840ae0f0d3bd13cf3df4d21dda2b1

Everything else you said sounds fine, Ivan. :)

This PR does not re-defines the JSON-LD context (and it should not), it uses a (in my view, bloated) context within the vocabulary definition in JSON-LD.

See my comment elsewhere: #920 (comment). Should I take that branch and merge with this one, to get this issue completely out of the way?

Of course, once this PR is settled, we have to look at the differences between the vocabulary and the v2 context to bring them in line. That is the next step, for sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think people are reacting to the fact that this PR re-defines the v2 JSON-LD Context...

Does it? I thought that was a totally different file that only the vocab used.

Exactly. And it uses it "internally", not for external usage.

Copy link
Member

@msporny msporny Sep 1, 2022

Choose a reason for hiding this comment

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

@dlongley wrote:

I thought that was a totally different file that only the vocab used.

Yes, I see that now... but it's not something the vocab uses, it's just a different representation of the human-readable vocabulary document. It took me a bit to remember what Gregg's ruby tool actually does (generates a human-readable and machine-readable vocabulary definition -- NOT a JSON-LD Context).

@iherman wrote:

And it uses it "internally", not for external usage.

Ah, I see now that the "JSON-LD Context" is just "the JSON-LD representation of the Vocabulary". I think both @OR13 and @mprorock were confused in the same way that I was.

Perhaps if we rename these files to vocabulary.(html|ttl|jsonld) it would lead to less confusion? Granted, actually reading the entirety of the file might have done that, but I do remember scanning down quite a ways... but not far enough to get to the vocabulary definitions at the bottom.

You can ignore all of my suggestions (except the one to rename the files) at this point. :P

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps if we rename these files to vocabulary.(html|ttl|jsonld) it would lead to less confusion? Granted, actually reading the entirety of the file might have done that, but I do remember scanning down quite a ways... but not far enough to get to the vocabulary definitions at the bottom.

You can ignore all of my suggestions (except the one to rename the files) at this point. :P

I am fine renaming the files; it is probably a good idea. I think I will also go ahead and simplify the json-ld file to the strict minimum to avoid any misunderstandings.

But... I have to call it a day, this will be done tomorrow...

@prefix owl: <http://www.w3.org/2002/07/owl#> .
@prefix rdf: <http://www.w3.org/1999/02/22-rdf-syntax-ns#> .
@prefix rdfs: <http://www.w3.org/2000/01/rdf-schema#> .
@prefix cred: <https://w3.org/2018/credentials#> .
Copy link
Contributor

Choose a reason for hiding this comment

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

how come we don't see

https://w3id.org/security#

In this list....

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the vocabulary does not refer/use this namespace.

Copy link
Member Author

Choose a reason for hiding this comment

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

(see also my comment in #920 (comment))

<section>
<h2>Introduction</h2>
<p property="dc:description"><%= ont["dc:description"]["en"] %></p>
<p>This specification makes use of the following namespaces:</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

I really hope we can simplify this list in v2.... especially removing odrl.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that this PR has not yet looked at the v2 version of the @context. In fact, it is only an attempt to clean up the 1.1 vocabulary.

I agree that the ODRL reference may be an overkill.

Copy link
Member

@msporny msporny left a comment

Choose a reason for hiding this comment

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

I believe the only blocking issue in this PR is the definition of vocab/credentials/v2/credentials.jsonld -- ideally, we should leave that (and all generated files), out of being checked in and perhaps generate them during the build process?

@iherman
Copy link
Member Author

iherman commented Sep 1, 2022

I believe the only blocking issue in this PR is the definition of vocab/credentials/v2/credentials.jsonld -- ideally, we should leave that (and all generated files), out of being checked in and perhaps generate them during the build process?

I am not sure what you say, @msporny. The credentials.json file is generated by a ruby script starting by the vocabulary csv file (just as for the HTML and the Turtle files). The script itself is generating too much insofar as, originally, it seems to have generated a vocabulary file and a @context file in one place. This became moot by the fact that the definition of the @context file is now done separately.

As I said, I have created a separate branch (simplify-json-ld), branching off this current branch, to simplify this. I have commented out some stuff from the ruby script which now generates a "pure" json-ld serialization of the vocabulary. Personally, I would like to use this in the final version.

@iherman
Copy link
Member Author

iherman commented Sep 2, 2022

@msporny @OR13 @dlongley, I have made the changes, including renaming the files themselves. Forget everything that you saw before 😀; here are the three files that are generated by the ruby script:

I hope this is now good to go/merge.

cc @pchampin

@msporny
Copy link
Member

msporny commented Sep 2, 2022

I am not sure what you say, @msporny. The credentials.json file is generated by a ruby script starting by the vocabulary csv file (just as for the HTML and the Turtle files).

What I'm saying is that if we have auto-generated files, we should use a Github process to auto-generate them instead of manually committing the files into Github. When we manually commit auto-generated files, there is a good chance that they will diverge from the underlying source files. It also unnecessarily pollutes the Github history with a bunch of check ins for auto-generated files. This doesn't have to be fixed in this PR, can be fixed in a future PR, and so we don't need to hold up the merge here to fix the longer-term issue.

Copy link
Member

@msporny msporny left a comment

Choose a reason for hiding this comment

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

Only one minor nit for a file that was probably checked in by mistake. Otherwise, LGTM.

Comment on lines 1 to 5
{
"files": [
{
"path": "vocab/credentials/v2/mk_vocab.rb",
"bookmarks": [
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't commit .vscode files for personal developer bookmarks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops... I usually have this on .gitignore, but I missed it this time. Sorry about that.

@msporny msporny self-requested a review September 3, 2022 12:41
@msporny
Copy link
Member

msporny commented Sep 3, 2022

Substantive, multiple reviews, changes requested and made, no objections, merging.

@msporny msporny merged commit 1789b95 into main Sep 3, 2022
@msporny msporny deleted the vocab-v2 branch September 3, 2022 12:43
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.

Data Model Vocabulary versioning questions Bug in the credential vocabulary specification
7 participants