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 representation language to address syntax and "unknown" properties. #454

Merged
merged 3 commits into from
Dec 1, 2020

Conversation

dlongley
Copy link
Contributor

@dlongley dlongley commented Nov 8, 2020

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

@dlongley
Copy link
Contributor Author

dlongley commented Nov 8, 2020

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.

@dlongley
Copy link
Contributor Author

dlongley commented Nov 9, 2020

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
Copy link
Contributor

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" ?

Copy link
Contributor Author

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.

Copy link
Contributor

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"?

Copy link
Contributor

@OR13 OR13 Nov 17, 2020

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.

Copy link
Member

@brentzundel brentzundel left a 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.

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

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
Copy link
Contributor

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.

Copy link
Member

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

Copy link
Contributor

@selfissued selfissued left a 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.
Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Member

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

Comment on lines -2378 to -2618
The value of the <code>@context</code> object member MUST be ignored as this is
reserved for JSON-LD consumers.
Copy link
Contributor

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."

Copy link
Member

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

index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
@burnburn
Copy link

burnburn commented Nov 17, 2020

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.

@dlongley
Copy link
Contributor Author

@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.

@w3c w3c deleted a comment from dlongley Nov 17, 2020
@msporny msporny mentioned this pull request Nov 17, 2020
@msporny
Copy link
Member

msporny commented Nov 24, 2020

Merge conflicts, please fix.

@msporny
Copy link
Member

msporny commented Nov 24, 2020

@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.

@iherman
Copy link
Member

iherman commented Nov 25, 2020

The issue was discussed in a meeting on 2020-11-24)

  • no resolutions were taken
View the transcript

3. 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.
… unsure if he is still objecting

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?

@dlongley dlongley force-pushed the dlongley-representation-syntax branch from ef49cd2 to 35e6c03 Compare December 1, 2020 18:24
@msporny
Copy link
Member

msporny commented Dec 1, 2020

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

@msporny msporny merged commit 3ddbe4b into master Dec 1, 2020
@dlongley dlongley deleted the dlongley-representation-syntax branch December 1, 2020 18:55
@iherman
Copy link
Member

iherman commented Dec 2, 2020

The issue was discussed in a meeting on 2020-11-01)

  • no resolutions were taken
View the transcript

2. PR 454

See 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.

Ivan Herman: +1

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.

Drummond Reed: +1

Drummond Reed: Agree with Brent.

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.

Daniel Burnett: I agree, as co-chair

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
… we are waiting on resolving your concerns

Manu Sporny: justin has attempted to add this additional language and defined terms
… context will be ignored, will not have additional processing

Michael Jones: is this the PR that deletes the text - we add other text to clarify what ignore means.
… all right it is equivalent to what we had before - well alright.

Drummond Reed: Yes, it is equivalent and actually clearer

Ted Thibodeau Jr.: note that the later note has a textual (and I believe editorial) tweak suggestion from me, which Github doesn't track as such, because you can't add a suggested change to a suggested change...

Manu Sporny: we added text around ignoring un-known properties - and it is more specific about what we mean. unrecognized properties will be preserved.

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.