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

Added tables to replace the introductory part on core properties - bis for github #536

Merged
merged 2 commits into from
Jan 17, 2021

Conversation

iherman
Copy link
Member

@iherman iherman commented Jan 11, 2021

resolving merge conflicts in #528 would have been terrible; git's rebase went completely crazy. However, my changes in #528 were fully concentrated to the initial part of §5, so I decided to make a fresh branch and manually transfer my changes instead.

@rhiaro, would you mind checking that the changes here are indeed the same as the changes proposed in #528 (except that they are on the fresh branch)? If so, @msporny, I believe the comments in #528 stand for this one, and this PR could be merged (and #528 simply closed).


Preview | Diff

…, fresh branch, to avoid horrific merge conflicts...
Copy link
Member

@rhiaro rhiaro left a comment

Choose a reason for hiding this comment

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

I've said it before, but this arrangement of the core properties is so much better and easier to read!

My one query is - do Verification Methods need to be referred to by DIDs (as stated here - but not in current spec text) or can they be any URIs?

And the caveat that once this is merged I will clean up the rest of the prose in the section so normative statements aren't duplicated. I also think we should use <dfn> tags around the properties in the table, but that can come alongside the prose cleanup.

(Changes below are editorial, sorry I didn't do them in the last one)

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@iherman
Copy link
Member Author

iherman commented Jan 11, 2021

@rhiaro thanks for the editorial comments; I have taken care of them.

As for

My one query is - do Verification Methods need to be referred to by DIDs (as stated here - but not in current spec text) or can they be any URIs?

yes, I was wondering about that myself. All examples in the text use DID URL-s for the various verifications methods but the spec is silent on this aspect altogether. This is one of the places when the reader might be influenced by the examples.

I have put a SHOULD for a DID URL to reflect the examples. If that is incorrect, then at least one examples, if not several, should use a different URI to make the point. Personally, I can live with either way.

@iherman
Copy link
Member Author

iherman commented Jan 11, 2021

(In case I was not clear: 41af738 covers the editorial changes.)

@rhiaro
Copy link
Member

rhiaro commented Jan 11, 2021

I have put a SHOULD for a DID URL to reflect the examples.

I checked with @msporny about this earlier, and I think the SHOULD here is correct.

@iherman
Copy link
Member Author

iherman commented Jan 14, 2021

@msporny can we/you merge this PR (and close #528)?

@msporny
Copy link
Member

msporny commented Jan 17, 2021

Editorial, multiple reviews here and in #528, changes requested and made, no objections, merging.

@msporny msporny merged commit c069c3b into main Jan 17, 2021
@msporny msporny deleted the vocabulary-tables-bis branch January 26, 2021 15:09
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.

3 participants