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

Fixed Credential Status Scheme #145

Closed
wants to merge 1 commit into from
Closed

Fixed Credential Status Scheme #145

wants to merge 1 commit into from

Conversation

David-Chadwick
Copy link
Contributor

@David-Chadwick David-Chadwick commented Mar 30, 2018

This is item v) from PR#141


Preview | Diff

@@ -645,7 +646,7 @@ <h2>Status</h2>
"ageOver": 21
},
<span class="highlight">"credentialStatus": {
"id": "https://dmv.example.gov/status/24,
"schemeID": "https://dmv.example.gov/status/24,
Copy link
Member

Choose a reason for hiding this comment

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

This breaks the standard JSON-LD data model. We have to keep "id".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is an inconsistency between the text and the JSON example. The text refers to a status scheme but the original JSON does not contain any mention of a status scheme. Closer alignment of the text and the JSON is needed so that the reader can better understand how the JSON is an example of the text.

Copy link
Contributor

Choose a reason for hiding this comment

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

Per the call, so the fix is to make the language say that "type" specifies the "type of scheme" and "id" identifies the specific instance of the scheme type, like a specific instance of a status list.

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.

If we move "SchemeID" back to "id", we can pull this PR in.

@burnburn
Copy link
Contributor

burnburn commented May 1, 2018

@David-Chadwick would it be okay to make the change to 'id' for JSON-LD reasons so we can merge and then you can open a new issue or PR regarding the attribute name?

@David-Chadwick
Copy link
Contributor Author

Yes OK. However the text would need to change to be consistent with the JSON. Can I suggest that it changes from this:

"The value of this property MUST be a status scheme that provides enough information to determine the current status of the credential (e.g. suspended, revoked, etc.). The status scheme will vary depending on a variety of factors, such as whether it is simple to implement or privacy-enhancing. "

to this

"The value of this property MUST be a credential status scheme that provides enough information to determine the current status of the credential (e.g. suspended, revoked, etc.). The credential status scheme is identified by the type and id properties, and will vary depending on a variety of factors, such as whether it is simple to implement or privacy-enhancing."

@msporny
Copy link
Member

msporny commented May 2, 2018

the type and id properties

Just be clear that id may not be required in all cases, but type is. That is id is a SHOULD, but type is a MUST.

@msporny
Copy link
Member

msporny commented May 16, 2018

Superceded by PR #176. Closing without merging.

@msporny msporny closed this May 16, 2018
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.

None yet

4 participants