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 the EnvelopedVerifiableCredential class to the vocabulary #1372

Closed
wants to merge 2 commits into from

Conversation

iherman
Copy link
Member

@iherman iherman commented Dec 4, 2023

This is a PR on top of PR #1358

To be merged by @msporny into that PR if acceptable...

Note: this PR does not include a change on the vocabulary diagram. If and when #1358 and #1370, I will do a separate PR with the necessary modification for both. Otherwise the drawio file will have ugly merge conflicts...

Copy link
Contributor

@OR13 OR13 left a comment

Choose a reason for hiding this comment

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

I prefer to no create a new term for this, if we can instead use "relatedResources"

@iherman
Copy link
Member Author

iherman commented Dec 5, 2023

I prefer to no create a new term for this, if we can instead use "relatedResources"

@OR13 please put this comment in the PR or issue that treats with the question directly (#1358 and/or #1352). This PR is a merely administrative PR to complete #1358.

@iherman
Copy link
Member Author

iherman commented Dec 7, 2023

In view of the latest direction taken by the WG (see, eg, #1358 (comment)), this PR is outdated. I have put a 'DO NOT MERGE' flag; I will have to make changes on the vocabulary to make it compatible with what is proposed now.

@iherman iherman changed the title Add the envelopedVerifiableCredential term to the vocabulary Add the EnvelopedVerifiableCredential class to the vocabulary Dec 7, 2023
@msporny
Copy link
Member

msporny commented Dec 7, 2023

PR #1358 has failed and has been marked as pending close. As a result, this PR is being marked as pending close.

An alternate attempt will be made here: #1358 (comment)

@msporny msporny added the pending close Close if no objection within 7 days label Dec 7, 2023
@iherman
Copy link
Member Author

iherman commented Dec 8, 2023

@msporny,

PR #1358 has failed and has been marked as pending close. As a result, this PR is being marked as pending close.

An alternate attempt will be made here: #1358 (comment)

To be clear: this PR has changed (hence the change of the title yesterday) and the current version introduces a separate EnvolopedVerifiableCredential class, to be used as a target for a type property in the solution outlined in #1358 (comment). I.e., technically, it would be possible to either take the changes into the new PR to come, or to rebase the current branch onto that PR.

But if, administratively, it makes it easier for you to start from fresh, I am fine closing this PR. As you prefer. The changes themselves are minor (adding a new class).

@msporny
Copy link
Member

msporny commented Dec 8, 2023

@msporny, To be clear: this PR has changed (hence the change of the title yesterday) and the current version introduces a separate EnvolopedVerifiableCredential class, to be used as a target for a type property in the solution outlined in #1358 (comment). I.e., technically, it would be possible to either take the changes into the new PR to come, or to rebase the current branch onto that PR.

Ah, I missed that. Removing the "pending close" and I'll try to base my new PR for EnvelopedVerifiableCredential on this PR.

@msporny msporny removed the pending close Close if no objection within 7 days label Dec 8, 2023
@TallTed
Copy link
Member

TallTed commented Dec 8, 2023

Maybe no longer a "DO NOT MERGE" PR?

@iherman
Copy link
Member Author

iherman commented Dec 9, 2023

Maybe no longer a "DO NOT MERGE" PR?

Actually, it should stay. For two reasons:

  1. This PR was raised "on top" of the https://github.com/w3c/vc-data-model/tree/msporny-enveloped-vc branch (which is the branch for Add mechanism to embed enveloped VCs in VPs. #1358). When I saw the new direction, I have updated this branch but, afterward, @msporny decided to close that PR in favor of a new branch that would come. At that point, this branch (ie, https://github.com/w3c/vc-data-model/tree/iherman-enveloped-vc-vocab) must either be rebased on top of the new branch to merge into that one (if that works) or the editor should take the editorial changes here and apply on the new branch.
  2. The class definition in the vocabulary has a reference to the VCDM spec for the formal definition of the class. I anticipated the URL (i.e., the fragment ID) to use, but that must be checked before any merge.

@iherman
Copy link
Member Author

iherman commented Dec 9, 2023

Ah, I missed that. Removing the "pending close" and I'll try to base my new PR for EnvelopedVerifiableCredential on this PR.

See #1372 (comment): I hope you mean to create a new PR based on 'main' and reuse this one. Do not start with this one, it would bring back https://github.com/w3c/vc-data-model/tree/msporny-enveloped-vc ☹️

@msporny
Copy link
Member

msporny commented Dec 9, 2023

PR #1379 has integrated this PR. Closing this PR in favor of PR #1379.

@msporny msporny closed this Dec 9, 2023
@msporny msporny deleted the iherman-enveloped-vc-vocab branch December 9, 2023 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
before-CR DO NOT MERGE PR contains something that should not be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants