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

add section to differentiate between contexts and credential Schemas #847

Merged
merged 20 commits into from
Feb 16, 2022

Conversation

kdenhartog
Copy link
Member

@kdenhartog kdenhartog commented Dec 6, 2021

@iherman
Copy link
Member

iherman commented Dec 8, 2021

The issue was discussed in a meeting on 2021-12-08

  • no resolutions were taken
View the transcript

2.2. add section to differentiate between contexts and credential Schemas (pr vc-data-model#847)

See github pull request vc-data-model#847.

Brent Zundel: Raised by Kyle, adding a section to differentiate between contexts and credential schemas. This adds a section to the informative appendix that says here's the base context and some text around what the difference is between a context and a schema..

David Chadwick: I haven't read it yet, apologies..

Brent Zundel: No worries, it's only been a couple of days, everyone read it and give feedback..

Manu Sporny: I think it's a good thing to put in the spec and it's purely editorial..

Brent Zundel: It looked good to me on my first glance through as well. I encourage folks to give feedback on that one..

@David-Chadwick
Copy link
Contributor

I dont like this text because it interweaves JSON-LD into the differentiation, whereas implementors who are using pure JSON will not be helped by this clarification. I think the clarification should do the following
a) describe how they are different in the absence of JSON-LD
b) optionally say what additional benefits JSON-LD can bring to either/both

@kdenhartog
Copy link
Member Author

kdenhartog commented Dec 8, 2021

That doesn't fall in line with the original issue that was trying to be resolved though. The original question that raised this issue was on the CCG mailing list linked in the issue and it was because people were confused about the specific differences between JSON-LD contexts and JSON schemas and what value they added to the data model. What you're proposing here seems to be a new issue not the same one that was raised in this PR.

@David-Chadwick
Copy link
Contributor

@kdenhartog Could you point me to the original issue please.
Regardless of this, people I have been working with have also raised the same issue with me. So I am looking for text that will help everyone who is unsure of the differences, not just those people who initially raised the issue.

@kdenhartog
Copy link
Member Author

The issue was #781 and the link to the email thread is on the CCG mailing list

FWIW, I don't disagree with what you're saying I'm just thinking that adding those concerns in via this PR may make this much slower to get the PR merged. Hence my preference would be to open a second issue where we can address those concerns separately based on my understanding of what you're concerned with and then find the best way to make additions in a separate section (that's my take of what you proposed is this would be another subsection of the Contexts and Credential Schemas appendix) and for specific updates to the text proposed here we can utilize the regular PR review process. Are you alright with taking that specific course of events to address both the original issue and the related but slightly tangential issue you're looking to address?

@David-Chadwick
Copy link
Contributor

@kdenhartog Thanks for the pointer. Having gone back and read the CCG email that you referred to, I dont believe that your current text is sufficient to address it, so I suggest that we keep this PR open until sufficient text can be added to it.

@kdenhartog
Copy link
Member Author

kdenhartog commented Dec 10, 2021

Hmm not sure we're not interpreting the email in the same way - This is the main focus of this PR:

Could folks please explain to me the uses of credentialSchemas in comparison to @context files in JSON-LD? Is it that @context files name the attributes and credentialSchemas provide the information about how to validate the data/semantics?

If you've got specific requests for things you believe still need to be addressed please do provide suggested text to add and we can discuss from there.

index.html Outdated Show resolved Hide resolved
@kdenhartog kdenhartog requested a review from msporny as a code owner December 15, 2021 23:50
@iherman
Copy link
Member

iherman commented Dec 16, 2021

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

  • no resolutions were taken
View the transcript

2.3. add section to differentiate between contexts and credential Schemas (pr vc-data-model#847)

See github pull request vc-data-model#847.

Manu Sporny: The only other one is 847.
… I think there was a question here and I didn't understand the resolution.

David Chadwick: I thought kdenhartog description was too narrow.
… I'd like the explanation to cover both.
... when I read the original email it seemed to imply it was not only in the context of JSON-LD, and I think that's the only concern there.

Manu Sporny: My reading is that it covers all of that already.
… so I'm confused.
… since it does cover JSON, JSON-LD, and SHACL.
… so my question is what should it do differently?.

David Chadwick: If you look at the text virtually every paragraph is about JSON-LD.
… so I don't object to it being about JSON-LD so I was hoping it would cover JSON only as well.
… so I would like to specify that context are there to cover "aliases".

Kyle Den Hartog: I'm happy if you provide some additional comments like that so I can put that in. When I read your description, it seemed like a much larger thing - paragraph or two talking about JSON, since it wasn't clear what you wanted in that text, easier to merge this and then have you add that text. If you're just talking about a sentence or two, then we can add it..
… However, if you have some suggested text, I can do that..

David Chadwick: The other thing I thought might be useful is type here.
… so I thought it would be useful to have another sentence about that.
… It might also be useful to bring in type... how does type differ from schema if the type tells you, why do you need schema?.

Kyle Den Hartog: That would be useful, mainly because type has a strong binding to how JSON-LD works and if you don't use types, things break. It should be useful in the context of typing having impact on semantics of data objects to data being dropped. With JSON Schema, you don't run into those problems, static checking of data model. There is usefulness in describing that, I will see if I can add something..
… Are you ok with that?.

Dave Longley: type and context provide semantics -- schema can be used to enforce shape.

David Chadwick: All we can do is try and make it better?.
… please make it noted that I'll add a clarifying sentence or two to this text.

Action #1: add a sentence or two to PR #847. (David Chadwick)

Brent Zundel: Any other comments on this?.
… we didn't cover V1.1 issues and I believe all but one are assigned.
… if you have one assigned please work on it.
… reminder next meeting is January 12th of next year.
… thanks everyone for coming.
… and thanks for the work you're doing and happy holidays.


Copy link
Contributor

@David-Chadwick David-Chadwick left a comment

Choose a reason for hiding this comment

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

Added more text and suggested altering some text

index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@kdenhartog
Copy link
Member Author

kdenhartog commented Jan 11, 2022

Your text in my opinion reduces this section to a factual redundant re-stating of the sections above about the @context, type, and credentialSchema property which strays away from the original issue at hand. The original issue was raised as a point to compare and contrast the two rather than to re-state what they do. It's clear based on the question having been raised on the mailing list (linked previously in the issue) that the current text does not adequately address the similarities and differences between the @context and the credentialSchema properties which is what I'd like to keep this PR focused on.

While I do believe the type component is useful, I wasn't a fan of the way you're looking to frame its purpose. I believe it's an important aspect to highlight to provide additional detail to the @context property and will put forward some additional text to help cover that as well. Thanks for this suggest.

I think some of your suggested texts are useful and I'll work to incorporate them to make the text I've added more clear. I've indicated with a thumbs up and thumbs down which texts will be incorporated. I'll take your text suggestions, make some modifications based on what I agree with and in the mean time we can continue to see what sort of resolution we can come to about the suggestions that I -1'd.

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

msporny commented Jan 12, 2022

We discussed this on the 2022-01-12 call, there is still disagreement on this PR that needs to be resolved before we can merge it.

@kdenhartog
Copy link
Member Author

kdenhartog commented Jan 12, 2022

@David-Chadwick and I are going to do some back and forth on this in a google document to get a better baseline of text that we can propose to the group. Let's put this PR on hold for now until him and I can get together a piece of text that can replace this PR for the WG to review. We're going to start from the most up to date text in this PR and work from there.

@iherman
Copy link
Member

iherman commented Jan 14, 2022

The issue was discussed in a meeting on 2022-01-12

  • no resolutions were taken
View the transcript

4. Review spec PRs.

Brent Zundel: We don't have a lot of time to discuss; if there is any more tweaking, it needs to happen soon.

Brent Zundel: https://github.com/w3c/vc-data-model/pulls?q=is%3Apr+is%3Aopen+label%3A%22v1.1+%28editorial%29%22+sort%3Aupdated-asc.

See github pull request vc-data-model#847.

@iherman
Copy link
Member

iherman commented Jan 14, 2022

The issue was discussed in a meeting on 2022-01-12

  • no resolutions were taken
View the transcript

4.1. add section to differentiate between contexts and credential Schemas (pr vc-data-model#847)

See github pull request vc-data-model#847.

Manu Sporny: By Kyle, this tries to add language to clarify difference between @context and credentialSchema.
… I asked for changes, David Chadwick asked for changes as well. David Chadwick you rejected this?.

David Chadwick: I don't know what the results of my proposed edits are. If my edits are accepted, I'd be +1.
… I got -1 from him, he doesn't want to add types, seems he is rejecting all my changes.

Manu Sporny: So we can't do anything with this without more discussion.

David Chadwick: He accepted one of my proposed changes.

Manu Sporny: I forgot to hit my "request changes" button..
… We can't pull it in, will pick it up later.
… Adding a comment to the PR about there still being disagreement.

Copy link
Contributor

@peacekeeper peacekeeper left a comment

Choose a reason for hiding this comment

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

I haven't really followed the conversation between @kdenhartog und @David-Chadwick, but I get this question about the difference between context and schema a lot. I believe this section is helpful to explain it and hope that some version of this can be agreed on and merged.

@kdenhartog
Copy link
Member Author

@msporny I've tried to include some of your suggestions in the latest round of revisions that David and I are working on. If anyone would like to track the progress of this work I've granted commenting access to anyone with the following link and will properly moderate for IPR and other considerations manually for now while we go back and forth on this a bit. @dlongley it would be good to get your take on the addition of the type paragraph I added in this document as well since I suspect you'll have some opinions or re-wording there.

https://docs.google.com/document/d/1obePJVDaJINPoUUPWN6fuN-d3o0ahbSpzUxiWncWXqA/edit?usp=sharing

@kdenhartog-mattr
Copy link

This text has been significantly updated since the previous reviews. Some people did track the work in the Google Document, but I believe it would be best to wait a bit longer on this text before merging it even though the 14 day period has lapsed.

@David-Chadwick this should be all the text updated to what you and I agreed to in the Google Document with some slight editorial changes to remove contractions. Please give it one last double check to let me know if there's any new issues I introduced by doing that.

@TallTed
Copy link
Member

TallTed commented Jan 28, 2022

I made a bunch of suggestions on the Gdoc, which remain pending. Please let me know if I need to make them on something here, now, instead.

@kdenhartog
Copy link
Member Author

I made a bunch of suggestions on the Gdoc, which remain pending. Please let me know if I need to make them on something here, now, instead.

Yeah having them here would be better now that we have the large chunks of text worked through the normal PR process should be suffice. I made some updates to the grammar and other editorial here which may have addressed some of your suggestions already as well.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
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.

Still some very minor nits with concrete suggestions. I think the content is fine at this point and we can merge after @kdenhartog has made a final pass and accepted the concrete suggestions.

@kdenhartog if you're the one to merge this, please either rebase/group the changes or squash the merge... we don't want a bunch of "Update index.html"s placed into the commit log.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
kdenhartog and others added 11 commits February 10, 2022 12:20
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Manu Sporny <[email protected]>
Co-authored-by: Manu Sporny <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
@kdenhartog
Copy link
Member Author

Ok @msporny and @TallTed I believe I've gone through and addressed every suggestion or comment raised in this PR as well as the GDoc. I think it would be good to get one last pass from you both as well as @TallTed

@msporny
Copy link
Member

msporny commented Feb 16, 2022

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

@kdenhartog kdenhartog merged commit 8f991da into v1.1 Feb 16, 2022
@kdenhartog kdenhartog deleted the kdh/issue-781 branch February 16, 2022 20:21
@iherman
Copy link
Member

iherman commented Feb 17, 2022

The issue was discussed in a meeting on 2022-02-16

  • no resolutions were taken
View the transcript

3.2. add section to differentiate between contexts and credential Schemas (pr vc-data-model#847)

See github pull request vc-data-model#847.

Kyle Den Hartog: larger text has seen significant review. comments addressed and have settled for 7 days. Manu says ready to merge if no other comments unaddressed. Ted's comments are in..
… any objections?.

Ted Thibodeau Jr.: +1 to merge.

Manu Sporny: have looked at this several times, but not most recent changes. will look at after the merge..

Kyle Den Hartog: your suggestions made it in, and Ted's. ready to merge. can fix things later if needed..

Manu Sporny: sounds great.

Kyle Den Hartog: will merge and squash..

Manu Sporny: could rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add details about difference between contexts and credentialSchemas property
8 participants