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

Vocabulary fixes #1241

Closed
wants to merge 1 commit into from
Closed

Vocabulary fixes #1241

wants to merge 1 commit into from

Conversation

OR13
Copy link
Contributor

@OR13 OR13 commented Aug 15, 2023

No description provided.

Copy link
Contributor

@awoie awoie left a comment

Choose a reason for hiding this comment

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

lgtm

@iherman
Copy link
Member

iherman commented Aug 16, 2023

The issue was discussed in a meeting on 2023-08-15

  • no resolutions were taken
View the transcript

1.8. Vocabulary fixes (pr vc-data-model#1241)

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

Brent Zundel: next PR is 1241 vocab fixes. seems straightforward. Orie do you want to speak to it?

See github issue vc-data-model#1240.

Orie Steele: copy-paste bug in the render method. question I had asked to Ivan on the value of the label property. related to issue linked (1240). slightly improves our vocabulary.

@iherman
Copy link
Member

iherman commented Aug 21, 2023

I approved the PR, but it seems to be incomplete.

  • If we introduce the cred:RenderMethod class as a range, then this class should be added to the vocabulary (it is currently not there). Probably as a 'reserved' class, just as the property itself
  • The vocabulary diagram is also incomplete: at this moment, nothing is indicated as a range for the property.

@OR13 what is your preference? I can take over the PR by adding these to the branch, I can issue a PR against this branch, or we would merge this PR and I will then step in with a new PR with these changes. Whatever you prefer.

Making a PR on top of this one turned out to be problematic, because the underlying main branch has changed too much with the addition of the diagram. It would have led to merge conflicts.

@OR13, I have created a new PR (#1253) which includes your proposed changes, plus the missing features mentioned above. Please check whether that is fine, and then we can close this PR without merge in favor of #1253.

@msporny @dlongley @longpd @decentralgabe @brentzundel @awoie @seabass-labrax

iherman added a commit that referenced this pull request Aug 21, 2023
@OR13 OR13 closed this Aug 21, 2023
msporny pushed a commit that referenced this pull request Aug 27, 2023
@msporny msporny deleted the fix/vocab-issues branch November 11, 2023 16:01
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.

9 participants