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

IssuanceDate and ExpirationDate #964

Closed
wants to merge 3 commits into from
Closed

IssuanceDate and ExpirationDate #964

wants to merge 3 commits into from

Conversation

mkhraisha
Copy link

@mkhraisha mkhraisha commented Oct 27, 2022

PR tackles #913
Updated Diagrams, Explainer, V2 context csv and used the npm generate script, removed the index.html deprecation warnings.


Preview | Diff

index.html Outdated
"holder": {"@id": "cred:holder", "@type": "@id"},
"issued": {"@id": "cred:issued", "@type": "xsd:dateTime"},
"issuer": {"@id": "cred:issuer", "@type": "@id"},
"issuanceDate": {"@id": "cred:issuanceDate", "@type": "xsd:dateTime"},
"validFrom": {"@id": "cred:validFrom", "@type": "xsd:dateTime"},
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that validUntil and validFrom are already present in the context, at the end -- so these are duplicates.

Copy link
Author

Choose a reason for hiding this comment

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

good catch, should be resolved now.

Copy link
Contributor

@decentralgabe decentralgabe left a comment

Choose a reason for hiding this comment

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

was just about to do this 😄

only question - do the existing context's need to be maintained for backwards compatibility?

@mkhraisha
Copy link
Author

was just about to do this 😄

only question - do the existing context's need to be maintained for backwards compatibility?

I believe so, this PR only affects v2.

@@ -17,9 +17,9 @@ property,credentialSchema,,,credential schema,,cred:VerifiableCredential,cred:Cr
property,credentialStatus,,,credential status,,cred:VerifiableCredential,cred:CredentialStatus,,The value of the `credentialStatus` property MUST be an instance of Credential Status.
property,credentialSubject,,,credential subject,,cred:VerifiableCredential,,,An entity about which claims are made.
property,evidence,,,evidence,,cred:VerifiableCredential,cred:CredentialEvidence,,The value of the `evidence` property MUST be one or more Credential Evidence instances.
property,expirationDate,,,expiration date,,cred:VerifiableCredential,xsd:dateTime,,The value of the `expirationDate` property MUST be a string value of an ISO8601 combined date and time string representing the date and time the credential ceases to be valid.
Copy link
Member

@msporny msporny Oct 28, 2022

Choose a reason for hiding this comment

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

Note that everything below here is going to conflict with https://github.com/w3c/vc-data-model/pull/968/files. You are either going to want to rebase on top of PR #968 or have @iherman rebase on top of your PR. Unfortunately, both of your PRs are making changes to a shared set of files. :)

@iherman
Copy link
Member

iherman commented Nov 2, 2022

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

  • no resolutions were taken
View the transcript

1.1. IssuanceDate and ExpirationDate (pr vc-data-model#964)

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

Manu Sporny: simultaneously ivan raised a PR to update vocabularies.

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

Manu Sporny: and they conflict with one another.
… Ivan and Mahmoud you have to talk to each other.
… that is the only thing that keeps those PRs from being merged.

Ivan Herman: my PR does two things. one yaml is now the language and the other thing it also includes the changes of property names.
… should have made that more clear.
… as far as i could understand it duplicates what Mahmoud has done.
… apologies for that.
… that is where we are.

Manu Sporny: that is great because we have more people doing PRs.

@iherman
Copy link
Member

iherman commented Nov 2, 2022

@mkhraisha I was looking at this in view of the conflicts with #968.

The conflicts are exclusively on the vocabulary part, ie, vocab/credentials/v2/vocabulary.* files (#968 does not touch anything else).

The problem is that #968 pretty much changes the way the ttl/jsonld/html files are generated. In the older version the starting point was the CSV file, whereas in the new version of the vocabulary generation the starting point is a YAML file. Also, I am not sure whether you changed the ttl/jsonld/html files by hand or you ran the generation scripts; if the former, that is a problem by itself, those files are generated.

#968 also takes care of the core issue of your PR, namely the new properties are introduced. Technically, that PR does not remove the old properties but, instead, deprecates them; because the vocabulary is already deployed, it is not a good idea to simply remove them from the vocabulary, thereby creating dangling links on the linked data world.

To move forward: would it be possible for you to roll back all the changes you have made on vocab/credentials/v2/vocabulary.*? (I am not sure technically what is the easiest way of doing so, @msporny, who is also a git-wizzard, may help in doing that.). If you are successful at that, then the conflicts disappear and the two PR-s can be merged independently of one another.

If that is too complicated, then another approach is that this PR is merged, and I can then either rebase my changes on (the new) main branch, or I simply remove #968 and create a new PR with the necessary changes.

Whichever is simpler for you (unless @msporny comes up with a better idea).

@mkhraisha
Copy link
Author

@iherman probably just easiest to close this PR and if the issue still persists after your PR is merged then I can recreate it. This was not a lot of work for me.
If that works please close it or let me know and I'll close it.

@msporny
Copy link
Member

msporny commented Nov 5, 2022

@iherman probably just easiest to close this PR and if the issue still persists after your PR is merged then I can recreate it. This was not a lot of work for me. If that works please close it or let me know and I'll close it.

Closing this PR in favor of #968. Once we merge #968, we can come back to this one.

@msporny msporny closed this Nov 5, 2022
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

5 participants