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

clarification of schema property #816

Merged
merged 7 commits into from Oct 27, 2021
Merged

clarification of schema property #816

merged 7 commits into from Oct 27, 2021

Conversation

David-Chadwick
Copy link
Contributor

@David-Chadwick David-Chadwick commented Sep 9, 2021

wyc and others added 3 commits July 13, 2021 16:59
* Add PR review process for 2021.

* Avoid GitHub id auto-linking.

Co-authored-by: David I. Lehn <dil@lehn.org>

* Update README.md

Co-authored-by: Manu Sporny <msporny@digitalbazaar.com>

Co-authored-by: Manu Sporny <msporny@digitalbazaar.com>
Co-authored-by: David I. Lehn <dil@lehn.org>
Co-authored-by: Brent Zundel <brent.zundel@gmail.com>
* Update presentation-graph.svg
* SVG coding changes
- make text into actual text
- use stylesheet
- simplify source for legibility
- Order similar to the flow, for accessibility
- Make credential-graph.svg a subsection, and note with a comment.
- Add slight background colouring to further delineate each subsection
* label the claim
As per comment from @TallTed
* differentiate graph labels
change font characteristics for graph-labels to distinguish them
(and make the stylesheet more explicitly obvious)
* Add desc (text alternative) in diagrams
This makes credentialgraph.svg and presentation-graph.svg more accessible as standalone diagrams.
* review comments
Copy link
Member

@kdenhartog kdenhartog left a comment

Choose a reason for hiding this comment

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

Good catch, this seems like an obvious change that just slipped through and won't cause any sort of breaking change. However, it does appear to be adding a normative statement so I'm going to classify it as a V1.2 change which I think is fine to go through during the maintainence WG.

@kdenhartog kdenhartog added errata Erratum for a W3C Recommendation maintenance issues that may be considered part of the work of the maintenance group substantive change v1.2 labels Sep 13, 2021
@David-Chadwick
Copy link
Contributor Author

There is an issue about whether a holder should insert a schema property into the VP that it issues, because there are going to be multiple flavours of VPs.
Finally, should we add a note to say that the schema is not intended to be constrained to defining only the properties of the credentialSubject (which some people have wrongly thought to be the case, even though I think the current text is clear that it applies to the entire VC)

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.

Fine with the concept, but no need to use normative language.

index.html Outdated Show resolved Hide resolved
@iherman
Copy link
Member

iherman commented Sep 15, 2021

The issue was discussed in a meeting on 2021-09-15

  • no resolutions were taken
View the transcript

3.4. clarification of schema property (pr vc-data-model#816)

See github pull request #816.

Brent Zundel: Next: PR 816

David Chadwick: I raised this because it was on a different list - TallTed asked me to point to it here.
… Some people are creating schemas that cover the credential subject and not the whole credential.
… I thought this could be because the text is not clear.
… First thing I found is that there is no statement that the credentialSchema must be present.
… But that would be a breaking change. So made it a "CAN".
… I think the text is clear that the schema property is for the entire credential.
… But there are now two other issues which I've put there: what about verifiable presentations?
… There could be different flavors of VPs... If the group agrees, we could put schema for VPs.
… The second issue: Should we put a note in, to specifically say that the schema is not intended to apply just to the subject.

Kyle Den Hartog: +1 I'm good with adding those to this PR

Brent Zundel: I am personally fine with those additional pieces of text being added to this PR, just because it is part of the same conversation we've been having.
… I'm not opposed to the addition of normative language if it's needed to address errata.
… I agree with Manu that in this case it would be difficult to say how this would be testable. Making a normative statement may not give us the assurances we are hoping for. And as you said, changing it to a "CAN" is fine.
… So the normative change would be to add a schema property for use in the VP. I think that is arguably errata and could be incorporated in the revisions we make, as long as it goes in this review period.
… Any questions, comments, before moving on to the next PR?

Manu Sporny: o/

@kdenhartog
Copy link
Member

I've relabeled this to a V1.1 change pending the update (change "MAY" to "can") in accordance with the discussion we had about this PR on the 9/15 call.

@David-Chadwick
Copy link
Contributor Author

I hope I have now accepted MAY to can using the web interface

@kdenhartog
Copy link
Member

kdenhartog commented Sep 27, 2021

Looks like it didn't go through quite properly when I just reviewed it, but I was able to get it all fixed up easy enough. Looks like everything should be good to go for this PR based on current feedback. Thanks for your addition @David-Chadwick

@kdenhartog
Copy link
Member

@msporny since you still have requested changes (I think they've all be addressed here at this point) I'll leave this one to you to merge.

index.html Outdated Show resolved Hide resolved
@wyc wyc changed the base branch from main to v1.1 September 29, 2021 15:34
@iherman
Copy link
Member

iherman commented Sep 29, 2021

The issue was discussed in a meeting on 2021-09-29

  • no resolutions were taken
View the transcript

1.4. clarification of schema property (pr vc-data-model#816)

See github pull request #816.

Wayne Chang: On issue 816, clarification of schema roperty, 3/4 approvals, manu requested changes, but seems like they've been addressed, is that correct?

Manu Sporny: If that was done, we're good to go.
… Yup, that's done, let me just approve, we're good.

Wayne Chang: Ok, squashing and merging.
… do we mean to merge into v1.1?
… Can I change this so it merges to v1.1?

David Chadwick: Yes, please.

Wayne Chang: Seems like they're conflicts, we'll have to fix those.
… Just README and diagrams are conflicting, this is more of a technical git issue, I can help w/ that.
… Any other comments on this PR

@David-Chadwick David-Chadwick mentioned this pull request Oct 6, 2021
@David-Chadwick
Copy link
Contributor Author

I have updated the README.md file (it appears that I needed to change two occurrences of "1" with "one". I am not sure if a) this is sufficient and b) whether I did it correctly or not. I am not sure what to do next

@kdenhartog
Copy link
Member

No worries, I was able to fix it for you easily. This is cause the main branch README.md is slightly different from the V1.1 branch readme, and I haven't got around to merging the changes from V1.1 back into main so that anyone building their feature branch off main won't encounter the same issues.

@kdenhartog
Copy link
Member

Multiple positive reviews, all feedback has been addressed, the 14 day merge period has been completed, and there's multiple positive reviews now. Will be updating the labeling so the errata points to the issue rather than the PR as well and then closing the issue.

@kdenhartog kdenhartog removed the errata Erratum for a W3C Recommendation label Oct 27, 2021
@kdenhartog kdenhartog merged commit 216c43d into v1.1 Oct 27, 2021
@kdenhartog kdenhartog deleted the schema branch October 27, 2021 21:22
kdenhartog added a commit that referenced this pull request Oct 28, 2021
Co-authored-by: David I. Lehn <dil@lehn.org>
Co-authored-by: Manu Sporny <msporny@digitalbazaar.com>
Co-authored-by: Brent Zundel <brent.zundel@gmail.com>
Co-authored-by: wyc <wyc@fastmail.fm>
Co-authored-by: chaals <chaals@users.noreply.github.com>
Co-authored-by: Kyle Den Hartog <kdenhartog@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance issues that may be considered part of the work of the maintenance group merge after 14 days
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants