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

Vocabulary fixes — replacement of #1241 #1253

Merged
merged 2 commits into from
Aug 27, 2023
Merged

Vocabulary fixes — replacement of #1241 #1253

merged 2 commits into from
Aug 27, 2023

Conversation

iherman
Copy link
Member

@iherman iherman commented Aug 21, 2023

This is actually a replacement of #1241 : it includes all the changes in there, plus the missing pieces in #1241 (comment).

Making PR on top of #1241 would have led to problems because, in the meantime, the main branch has changed too much (e.g., the diagrams have been added), so it was simpler to create a PR on top of main rather than fix/vocab-issues

@OR13
Copy link
Contributor

OR13 commented Aug 21, 2023

This PR does not include the correction to credential graph. A credential graph is not a credential.

@iherman
Copy link
Member Author

iherman commented Aug 21, 2023

This PR does not include the correction to credential graph. A credential graph is not a credential.

The vocabulary file on the main branch already has this change:

label: Verifiable credential graph

Copy link
Contributor

@decentralgabe decentralgabe left a comment

Choose a reason for hiding this comment

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

@iherman should this include w3c/vc-json-schema#206 as well?

Copy link
Contributor

@OR13 OR13 left a comment

Choose a reason for hiding this comment

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

@iherman can you please update the label for this section of the vocabulary? That is the change that is not carried over from the previous PRScreenshot_20230821-123925.png

@OR13
Copy link
Contributor

OR13 commented Aug 21, 2023

Similarly for the credential graph label... The labels should distinguish the concepts. It's not helpful to call everything simply a "verifiable credential"... When it could be a set of separate credential graphs or a Verifiable Credential that is not a Verifiable Credential Graph.

@iherman
Copy link
Member Author

iherman commented Aug 22, 2023

@iherman can you please update the label for this section of the vocabulary? That is the change that is not carried over from the previous PR!
Screenshot_20230821-123925.png

Oops, sorry. I mixed it up. Done.

@iherman
Copy link
Member Author

iherman commented Aug 22, 2023

Similarly for the credential graph label... The labels should distinguish the concepts. It's not helpful to call everything simply a "verifiable credential"... When it could be a set of separate credential graphs or a Verifiable Credential that is not a Verifiable Credential Graph.

I do not understand what you want to say. Is there a change you propose in the vocab file? If so, can you put that explicitly in the comment?

@@ -163,7 +171,7 @@ property:

- id: verifiableCredential
defined_by: https://www.w3.org/TR/vc-data-model-2.0/#defn-verifiableCredential
label: Verifiable credential
label: Verifiable credential graph
Copy link
Contributor

Choose a reason for hiding this comment

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

this was the change I was looking for.

Copy link
Contributor

Choose a reason for hiding this comment

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

@iherman I was doing code reviews from my phone, its possible I missed this in your original PR.

The general comment was to address the labels and try to provide unique language that distinguished all the various "verifiable credential" labels.

It seems like my feedback has been addressed.

@seabass-labrax
Copy link
Contributor

I agree with the discussion in the 2023-08-22 meeting that this PR is substantially similar to @iherman's earlier PR and so should inherit that PR's approvals.

@msporny
Copy link
Member

msporny commented Aug 27, 2023

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

@msporny msporny merged commit dbd67ab into main Aug 27, 2023
1 check passed
@msporny msporny deleted the fix/vocab-issues-ih branch August 27, 2023 23:25
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.

7 participants