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 Metadata section based on infra data structures. #347

Merged
merged 3 commits into from Jul 28, 2020

Conversation

msporny
Copy link
Member

@msporny msporny commented Jul 14, 2020

This PR attempts to write some base text for the metadata section based on infra data structures. It attempts to strike the right balance by strongly suggesting that string-string values should be used. However, if non-string values are required, then infra types that are loss-lessly expressible across all representations must be used. Hopefully this strikes the right compromise.


Preview | Diff

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.

This seems good, suggest strings, allow maps, lists, etc... seems like the best of both worlds.

@brentzundel brentzundel added the metadata issues and PRs related to metadata label Jul 16, 2020
Copy link
Contributor

@selfissued selfissued left a comment

Choose a reason for hiding this comment

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

Use simple, general, descriptive language for the values. Do not suggest what the type will be.

OAuth metadata includes, strings, booleans, arrays, etc. There's no reason to believe that DID metadata will be otherwise.

index.html Outdated
<a>DID URL Dereferencing</a>, and other DID-related processes. The structure
used to communicate this metadata MUST be a <a data-cite="INFRA#maps">map</a> of
properties. Each property name MUST be a <a data-cite="INFRA#string">string</a>.
Each property value SHOULD be a <a data-cite="INFRA#string">string</a>. In the
Copy link
Contributor

Choose a reason for hiding this comment

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

The "SHOULD be a string" language is the worst of both worlds. It encourages developers to write code that assumes strings, that will then break when other object types are used. Please delete the prejudicial text.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jricher we're waiting on feedback from you on this one. The discussion has boiled down to your position vs. @selfissued's position. I made an attempt at a compromise, which doesn't work for @selfissued, so it'll be up to both of you to hash out what you want the "Each property MUST/SHOULD be a string. " sentence to state... or if the sentence should be deleted entirely.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that "SHOULD" is really bad in this case as it will lead to premature optimization on the part of consumers, which will break in the way that Mike recommends. My goal was to separate the data structure of the metadata from the serialization of the DID document itself. They should not be tied together, and the reference to core representations here is misleading.

Copy link
Member Author

Choose a reason for hiding this comment

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

Applied suggestions from @jricher, looks like we have consensus as @selfissued seems to agree in the comments. Not seeing any objections.

index.html Outdated
data-cite="INFRA#lists">list</a>, <a data-cite="INFRA#boolean">boolean</a>,
number, or <a data-cite="INFRA#null">null</a> and it MUST be possible to
losslessly represent the value across all supported representations as
described in <a href="#core-representations"></a>.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please simplify the text by saying that any representable value, such as strings, booleans, arrays, objects, and nulls may be the value.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jricher waiting on feedback from you on this one too. Need you to hash this out with @selfissued.

Copy link
Member Author

Choose a reason for hiding this comment

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

Applied suggestions from @jricher, looks like we have consensus as @selfissued seems to agree in the comments. Not seeing any objections.

index.html Outdated
<a>DID URL Dereferencing</a>, and other DID-related processes. The structure
used to communicate this metadata MUST be a <a data-cite="INFRA#maps">map</a> of
properties. Each property name MUST be a <a data-cite="INFRA#string">string</a>.
Each property value SHOULD be a <a data-cite="INFRA#string">string</a>. In the
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that "SHOULD" is really bad in this case as it will lead to premature optimization on the part of consumers, which will break in the way that Mike recommends. My goal was to separate the data structure of the metadata from the serialization of the DID document itself. They should not be tied together, and the reference to core representations here is misleading.

index.html Outdated
Comment on lines 3475 to 3477
The following example demonstrates a JSON-encoded metadata structure sent to
the <a href="#did-resolution">DID Resolution process</a>.
</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

The resolution process should absolutely NOT accept something "JSON-encoded". It should accept a "map". Serialization is internal to the process, on either input or output from the function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Applied your suggestions verbatim.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
Co-authored-by: Justin Richer <justin@bspk.io>
@msporny
Copy link
Member Author

msporny commented Jul 28, 2020

Normative, multiple reviews, changes requested and made, no objections, merging.

@msporny msporny merged commit 3d447e6 into master Jul 28, 2020
@msporny msporny deleted the msporny-metadata-infra branch July 28, 2020 13:18
@msporny
Copy link
Member Author

msporny commented Jul 28, 2020

For what it's worth, I'm exploring how we add "number" to Infra here: whatwg/infra#87

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
metadata issues and PRs related to metadata
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants