-
Notifications
You must be signed in to change notification settings - Fork 106
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
Warn that using dynamic processor features might lead to less interoperability #1276
Conversation
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.
are there any static validation tools implementers can use to not footgun themselves? maybe better suited for an impl guide...but worth having
I think I preserved intended meaning. Negation of negative negation is challenging. |
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 still does not address what the value of the non compact JSON-LD is... and if that value is not elaborated on substantially, we should recommend no JSON-LD processing at all.
I would like to see language that specifically addressed application n-quads, or JSON-LD with @graph
I recognize it might be better addressed in the value of JSON-LD PR. Before we add more text saying you don't need to do JSON-LD processing, and should avoid creating problems when JSON-LD processing occurs we need to address the value of JSON-LD processing, and in particular the value of RDF... which is ultimately what JSON-LD is about. |
The issue was discussed in a meeting on 2023-09-14
View the transcript1.7. Provide guidance to use compact terms/types instead of full URLs (pr vc-data-model#1276)See github pull request vc-data-model#1276. Brent Zundel: Few requests for changes from TallTed. Not a lot of review yet, raised 2 days ago. Manu Sporny: This is in response to an issue that DavidC raised. Brent Zundel: Please review folks. |
index.html
Outdated
<li> | ||
Always using the compact form for JSON-LD terms and types and always ensuring | ||
that those terms and types are defined in application-specific JSON-LD context | ||
files. |
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.
Suggest replacing the above three lines by:
Always using the compact form for JSON-LD terms and types when these are defined in JSON-LD context files.
Application specific terms and types MAY use the long (URI) form when the compact forms are not defined in any JSON-LD context file.
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.
@David-Chadwick -- I'm confused by your comments. Note that the Use ... files
in green in my change request, which replaced the three lines you're suggesting be replaced by something else, is an item in unordered list of "avoid using these JSON-LD features" ... and you seem to be saying "don't tell implementers to use JSON-LD context files" which is already happening. Part of my confusion stems, I think, from your having made 3 comments on the same bullet point, which comments don't seem to be fully connected thoughts, nor to take my existing change request into consideration.
My sense is that you're reading the PR Preview, which doesn't reflect suggested changes until/unless the suggestions are merged.... Please take another look at the HTML source, and base your suggestion on the total context of that unordered list, starting with line 3991 and continuing to line 4011?
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.
@TallTed Your change request is just that, a request. It has not been merged nor accepted. When it has been I can comment on it. But until then my review is based on the text that is displayed to me when I perform the review. This is the text I am asked to review. So your sense that I am reading the PR Preview is correct.
However I have also looked at your change request and it does not appear to change the semantics of the original text, as it appears to only remove the word 'always'.
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.
[@David-Chadwick] it appears to only remove the word 'always'
I strongly suggest that you re-examine my change request for that paragraph (bullet point), and tell me again whether it appears to only remove the word 'always'. If so, I will have little choice but to ignore your comments, as you are not responding to the full flesh of my own; indeed, you're acting as if my change requests do not exist.
PRs on GitHub are conversations. PR Preview is a helper but it only shows the result of the PR as initially drafted, and as/when conversation on the PR results in revision of the PR (which is to revise the text of the document). Ignoring the conversational aspect of this tool does a disservice to all involved, yourself included.
(To be most explicit: My change request significantly rewords the content of that bullet, which is to make it make sense in context of my small rewording of the intro to those bullets, along with my suggested changes to the other bullets in the list, which are to make them all agree with one another. Your own change only applies to the single bullet, not to the intro, nor the other bullets, and does not appear to me to help in bringing clarity to this segment of the document.)
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.
@David-Chadwick wrote:
Always using the compact form for JSON-LD terms and types when these are defined in JSON-LD context files.
This seems fine.
Application specific terms and types MAY use the long (URI) form when the compact forms are not defined in any JSON-LD context file.
Hrm, but we have a base @vocab
that makes it so they don't have to do that. I believe there were objections in the group to using long-form URLs and adding the text you suggest above would re-raise those objections. @OR13, I believe you were one of the people that wanted clear guidance here -- thoughts?
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.
Sorry, I escaped the thread: #1276 (comment)
The linked comment is probably a better way to address this issue.... the default behavior should not appear to be illegal, imo.
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.
Application specific terms and types MAY use the long (URI) form when the compact forms are not defined in any JSON-LD context file.
The reason for adding this text is that it still allows compact local names to be used because of the @vocab
term, but it also allows implementors who wish to use a global long (URI) name to use that as well, ensuring that no other VC Issuer will have a name clash with their invented compact local name when they do not want to start creating their own @context
files
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've attempted to address this issue in the latest series of edits, please re-review @David-Chadwick.
@msporny I think this comment addresses the impact that ^ if folks don't understand what the |
The issue was discussed in a meeting on 2023-10-11
View the transcript1.13. Provide guidance to use compact terms/types instead of full URLs (pr vc-data-model#1276)See github pull request vc-data-model#1276. Brent Zundel: Guidance for the use of compact terms and types rather than full URLs. I know this is related to the light processing PRs. Manu Sporny: There's an issue raised to provide clear guidance on using compact terms vs. full URLs. I think Orie is saying that this PR doesn't address this issue and it might be better address in the value of JSON-LD PR. I don't know what is needed to make it so that Orie is ok with it. It is a PR that would be closed because we couldn't get to consensus. Brent Zundel: Thank you, Manu. We're at time for the meeting. Appreciate folks joining, kinda crazy with people at IIW, etc. |
@TallTed @OR13 @David-Chadwick Your change requests have resulted in this PR being marked as pending close. If you feel you can save this PR w/ change suggestions, please do, if not, the PR will be closed and the issue will go unresolved in this iteration of the WG. |
index.html
Outdated
</li> | ||
<li> | ||
Use of full URLs for JSON-LD terms and types, instead of the compact forms of | ||
those values as are defined in application-specific JSON-LD context files |
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.
What about
"instead of the compact forms of those values whether or not they are defined in application-specific JSON-LD context files"
This covers the cases when a new context file is created or when a new context file is not created.
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.
@David-Chadwick — There is no "compact form" of such URL values, if they're not mapped (I think "defined" remains the wrong verb for what @context
files do, whether I accidentally re-introduced it or overlooked it, in a relevant comment or suggestion related to this thread) by an @context
file, no matter whether that file is old, new, generic, application-specific, etc.
The thing called "compact form" here — probably better to be called the "unmapped form" — is the naked word, the non-URL. The full URL is what that "unmapped form" is mapped to by some @context
identified by the JSON-LD document in question, which is probably one of the documents cited in the VC specifications, unless the "unmapped form" in question is not mapped by any of those VC @context
files.
These "unmapped forms" might be validly referred to as "JSON keys" given that a JSON file contains some number of key/value pairs; and a JSON-LD file contains some number of key/value pairs where the JSON-LD-relevant keys are mapped by a file identified by the @context
property, and some additional number of key/value pairs involving keys that remain un-mapped by any identified @context
are ignored as non-conforming or uncrecognized.
those values as are defined in application-specific JSON-LD context files | |
those values as are mapped by application-specific JSON-LD context files |
Beyond that, I don't believe I drafted these bullets nor their introductory paragraph, though I did content-edit both, so I'm hoping earlier author(s) will chime in on whether they see their meaning as being preserved, lost, clarified, distorted, etc., by current efforts.
index.html
Outdated
</li> | ||
<li> | ||
Using full URLs for JSON-LD terms and types instead of the compact form of those | ||
values defined in application-specific JSON-LD context files |
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.
Replace with
"Use of full URLs for JSON-LD terms and types, instead of the compact forms of
those values which may or may not be defined in application-specific JSON-LD context files"
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 makes no sense in context of the existing thread (started by your 19 hour older comment, followed on by my 10 hour older comment). The verb tense here ("Using") breaks from the intro preceding the <ul>
and differs from that in both bullets including this one. I had much to say about your "values which may or may not be defined"/"values whether or not they are defined" in my previous comment, but that comment appears to have been ignored completely.
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 get your point. But I am not sure you get the point I am trying to make, which is: an issuer that only uses the default at context files, but defines their own application types, wishes to use the full URLs for these, since the VCDM requires them to be URLs. The issuer does not want to use a local string that maps via the at vocab into a URL, since another issuer could use the same local string that maps into the same URL via the at vocab, whilst it has a different semantic. Using the full URL is conformant to the VCDM but the proposed text says that it might affect interoperability. However using a local string could cause a bigger car crash because recipients would interpret two different application 'local string' types to be the same one by mapping them both via the at vocab.
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 I understand what you're trying to say here. I think it may not be possible (nor necessary) to wedge all that nuance into this list item. I have a new draft for the list item, which changes both existing lines, so it's not going directly on this thread (which is only attached to one line of text). I suggest trying to craft a new list item, if the existing list items do not now cover your concern as elaborated above.
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.
Alternatively we could change the text of this PR to be more precise, which you could help by wordsmithing the following e.g.
"using full URLs for JSON-LD terms and types instead of the compact form of those
values that are either explicitly defined in application-specific JSON-LD context files, or are implicitly defined by the @vocab
feature of the base @context
file https://www.w3.org/ns/credentials/v2
The issue was discussed in a meeting on 2023-10-31
View the transcript1.11. Warn that using dynamic processor features might lead to less interoperability (pr vc-data-model#1276)See github pull request vc-data-model#1276. Brent Zundel: This one is already marked pending close. Manu Sporny: The reason it was marked pending close is because Orie was objecting but just didn't use the button to object. I don't see a way around ... we need Orie to respond or it's getting closed and I expect closure.
Brent Zundel: I appreciate Dave pointing out that this is a post-CR PR. Dave Longley: If I recall, Orie's objection was around recommending that people don't use certain JSON-LD features. I think we've changed the text to say it's not the goal... we want to inform people that if they use a lot of features, they might get less interop. Brent Zundel: Thank you, Dave, I appreciate that comment, I've marked it post-CR. |
index.html
Outdated
<li> | ||
Use of full URLs for JSON-LD terms and types (e.g., | ||
`https://www.w3.org/2018/credentials#VerifiableCredential`), instead of the | ||
unmapped forms of any such values (e.g., `VerifiableCredential`) that are |
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.
(e.g., https://www.w3.org/2018/credentials#VerifiableCredential
or https://example.org/somenewtype
) instead of the unmapped forms of any such values (e.g., VerifiableCredential
or somenewtype
) that are
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.
@David-Chadwick -- "that are" what? Your comment above seems to have been cut off.
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.
no its not. This is a replacement for lines 3905 and 3906 so that 3907 continues as before
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.
NOTE: The below is not my change suggestion, and I do not support it. I would draft further changes that might help, if it is re-suggested using the tools as designed. — @TallTed
@David-Chadwick — Ah! Your comment started with (e.g.,
which made me think it was starting a new thought, because no line above starts with (e.g.,
.
When you're making a change suggestion, there's a toolbar, with an ± icon as highlighted below.
If you click that, it will copy the lines you've selected to comment on to the text entry box, within a suggestion
block -- that is, with a leading line of —
```suggestion
— and a closing line of —
```
— and if you edit those lines to be what you want to suggest, something like this —
```suggestion
(e.g.,
`https://www.w3.org/2018/credentials#VerifiableCredential` or `https://example.org/somenewtype`) instead of the
unmapped forms of any such values (e.g., `VerifiableCredential` or `somenewtype`) that are
```
— then future readers will see something like this (now that I understand what you were trying to say), with the existing text directly above your suggested text, with deletions highlighted in red and additions highlighted in green —
unmapped forms of any such values (e.g., `VerifiableCredential`) that are | |
(e.g., | |
`https://www.w3.org/2018/credentials#VerifiableCredential` or `https://example.org/somenewtype`) instead of the | |
unmapped forms of any such values (e.g., `VerifiableCredential` or `somenewtype`) that are |
Now, this only shows line 3906 as being changed, because you only selected that line for your comment. Next time, try click-and-hold on 3904 (where the (e.g.,
is found) and then drag down to 3906 (which ends with that are
), to clearly suggest changes to those three lines.
If you use this tool as designed, you'll find your suggestions are much clearer to your readers.
NOTE: The above is not my change suggestion, and I do not support it. I would draft further changes that might help, if it is re-suggested using the tools as designed. — @TallTed
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.
Thankyou. I will try this next time. I was not aware of the feature. I only thought you could edit line by line
index.html
Outdated
`https://www.w3.org/2018/credentials#VerifiableCredential`), instead of the | ||
unmapped forms of any such values (e.g., `VerifiableCredential`) that are | ||
among active JSON-LD `@context` mappings (e.g., | ||
`https://www.w3.org/ns/credentials/v2`) |
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.
add
" through either explicit mappings or the @vocab
feature that covers all other unlisted values"
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.
[@David-Chadwick ] add
" through either explicit mappings or the @vocab feature that covers all other unlisted values"
I don't understand what you intend to communicate by this addition, so I cannot suggest revision. My inability to decipher your meaning might suggest a need for rephrasing such that others — who do not have even the benefit of our discussions to date — to understand it.
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.
It would be helpful to use an example of the problem that I am raising
Please do provide such an example! |
@TallTed Can you please wordsmith this to be precise
|
The issue was discussed in a meeting on 2023-11-14
View the transcript1.2. Warn that using dynamic processor features might lead to less interoperability (pr vc-data-model#1276)See github pull request vc-data-model#1276. Brent Zundel: this is post-cr, so would like to understand status today. Manu Sporny: I believe DavidC was asking for changes and manu requested and received re-review with some subsequent discussion. David Chadwick: I did produce the text, but TallTed had suggested reformat as a block. Ted Thibodeau Jr.: that explanation is helpful, but I don't see that communicated in the current text. David Chadwick: I had some difficulty with the GitHub UI trying to add multiple lines to the change request. Manu Sporny: Can we change this one to pre-cr? I've been ignoring post-cr issues, but this one seems like it may have a resolution this week. Brent Zundel: It might make sense to do that, yes. |
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Dave Longley <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
fb20a48
to
39b4df4
Compare
@David-Chadwick I've edited your content here: #1276 (comment) |
Editorial, multiple reviews, changes requested and made, no objections, merging. |
This PR attempts to address issue #1206 by warning document authors that using dynamic JSON-LD processor features, such as full URLs like
https://schema.org/Person
for terms/types, might lead to less interoperability with static JSON-LD processors.Preview | Diff