-
Notifications
You must be signed in to change notification settings - Fork 99
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 representation language to address syntax and "unknown" properties. #454
Conversation
This PR may need some further tweaks/rebasing on another PR that separates properties in the data model that carry representation-syntax from information properties (with information related to the DID subject). I'll coordinate with @msporny on that restructuring as needed. |
See #455 for reworked text around property types. This can be rebased on top of that PR when appropriate. |
index.html
Outdated
@@ -1162,6 +1162,11 @@ <h2>Representations</h2> | |||
are conformant with the fragment processing rules defined in section | |||
<a href="#fragment"></a> of this specification. | |||
</li> | |||
<li> | |||
The representation MAY define representation-specific syntax that can |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need a definition for "representation-specific syntax" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may get editorially adjusted to "representation properties" (or whatever language is settled on there) based on PR #455.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is CDDL "representation-specific syntax"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jonnycrunch based on the relationship between CDDL and mime types, I would say that CDDL is used to express representation specific syntax, that is more compatible with a comprehensible ADM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some editorial suggestions, otherwise LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment here #455 (review). I agree this reflects the decisions during the F2F. Still, for the record, it doesn't feel great to call representation-specific syntax "properties" in the same way as verification methods, services, etc. are "properties".
<p> | ||
Consumers MUST add all properties that do not have explicit processing rules | ||
for the representation being consumed to the abstract data model using only the | ||
representation's generic type processing rules. Producers MUST serialize all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Abstract Data Model is too Abstract. We need to make this more concrete before enforcing the MUST.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we did that with #455
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cited text from two resolutions cited that needs to appear in the PR, for it to do the jobs that the working group agreed to. Thank you.
</p> | ||
|
||
<p> | ||
Unknown object member names MUST be ignored as unknown properties. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please replace the deleted text above with the text of the resolution https://www.w3.org/2019/did-wg/Meetings/Minutes/2020-11-03-did#resolution2 : "Unrecognized properties MUST be preserved." If this is not the right place, then put it somewhere else. But the text of the approved resolution needs to normatively appear in the spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the spirit of that resolution may be captured by this text, which is proposed above:
Consumers MUST add all properties that do not have explicit processing rules
for the representation being consumed to the abstract data model using only the
representation's generic type processing rules. Producers MUST serialize all
properties in the abstract data model that do not have explicit processing
rules for the representation being produced using only the representation's
generic type processing rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this suggestion addresses your concerns: #454
The value of the <code>@context</code> object member MUST be ignored as this is | ||
reserved for JSON-LD consumers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please replace the text above with that approved in the resolution https://www.w3.org/2019/did-wg/Meetings/Minutes/2020-11-05-did#resolution2 : "Note that the value of the @context object member will be ignored, if present."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This suggestion may address this concern: #454
Apologies all (and particularly @selfissued @dlongley), I misread resolution #2 during today's call. We absolutely agreed, in that resolution, to add an informative note about the effect on @context, as the text of the resolution specifically says, independent of the normative language we are properly adding elsewhere. In fact, I was the one to propose that specific text wording be added. And yes, it is legal to include informative notes in normative sections of specifications (which was the discussion Ivan and I had on the f2f call on the adding of this specific text). I believe this is addressed by Brent's suggested change. I would be fine with Justin's version (with addition) as well, but his added text is not strictly required in order to fulfill the resolution as stated. |
@burnburn, I don't think anyone is opposed to the note, the issue is with the language (we want to avoid reintroducing the ambiguous "perserved"/"ignored" language in the note). I didn't want it to hold up this PR while we figure out exactly what the note language should be or where the note should go at this point. However, since that ship has sailed and it's holding things up now, there's a proposed change with a note in it that can be further editorialized in the future. Hopefully this gets us to consensus and the PR can be merged. |
Merge conflicts, please fix. |
@selfissued can you please respond to the people that are asking you questions in this PR? At this point, there is general agreement to merge it -- you're the one that everyone is waiting on to make progress on this PR. |
The issue was discussed in a meeting on 2020-11-24)
View the transcript3. PR 454 and 455...See github pull request #454, #455. Brent Zundel: status update? dlongely: we are waiting for a response from mike jones regarding the suggestions for adding a note to the PR / regardign @context in the JSON representation section. Brent Zundel: I will ping him again Markus Sabadello: after merging 455 and then 454 we still need to revisit the exact terminology... lots of different properties type to hammer down. See github issue #463. Markus Sabadello: feedback welcome Brent Zundel: other PRs that need to be discussed in this meeting? |
…about unknown properties. Co-authored-by: Justin Richer <[email protected]>
ef49cd2
to
35e6c03
Compare
Normative, multiple reviews, changes requested and made, no objections, merging. |
The issue was discussed in a meeting on 2020-11-01)
View the transcript2. PR 454See github pull request #454. Daniel Burnett: main item for today pull request 454 Daniel Burnett: Manu where are we Manu Sporny: we are waiting for mike jones to tell us if he is happy with it or not Daniel Burnett: we had some time waiting for proposals. should we move on to priority 1 & 2 issues and come back go 454 (10 min at the end) … 5. Back to 454{: #section5} See github pull request #454.
Manu Sporny: we need a position on 454 my suggestion we merge it in - any changes mike wants can be made in a new PR.
Brent Zundel: essentially same things as manu - a number suggestions in the PR that are good faith efforts to meet mikes concerns those should be merged.
Manu Sporny: editors can proceed. joe: my question is address by drummond's comment Ivan Herman: Mike has arrived. Manu Sporny: Mike, we're discussing 454 with the changes that have been suggested would you be ok with merging the PR. Daniel Burnett: chairs believe there is a good faith effort Manu Sporny: justin has attempted to add this additional language and defined terms Michael Jones: is this the PR that deletes the text - we add other text to clarify what ignore means.
Manu Sporny: we added text around ignoring un-known properties - and it is more specific about what we mean. unrecognized properties will be preserved. |
Here's an attempt at getting generic/unknown/syntax-related property representation rules into the spec per the resolutions/discussions this past week.
Preview | Diff