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

Introduction of a canonical manifest #306

Merged
merged 4 commits into from
Aug 21, 2018
Merged

Introduction of a canonical manifest #306

merged 4 commits into from
Aug 21, 2018

Conversation

iherman
Copy link
Member

@iherman iherman commented Aug 15, 2018

This PR will, eventually, serve as a replacement for the lifecycle section in the draft.


Preview | Diff

@iherman
Copy link
Member Author

iherman commented Aug 15, 2018

As a first step, I have created a separate section for the creation of a canonical manifest, as proposed discussed in #268.

@iherman
Copy link
Member Author

iherman commented Aug 15, 2018

@HadrienGardeur, good point (in
#268 (comment)) on the term names with a "@" vs. WebIDL.

A problem I can foresee is when the WebIDL is mapped against a specific language like JavaScript. It would make it a bit messy, because you cannot use the "dot" syntax for object attributes with a "@", which is a bit of a problem.

I am not sure what the best approach to handle this is. We could define, for all the "@" terms, a "@"-less version in the context file, allow the usage of both, and make the "@" disappear in the canonical manifest. Or simply use only the "@"-less versions in our manifest, too.

(To be checked whether the schema.org tools would understand types if used that way.)

@iherman
Copy link
Member Author

iherman commented Aug 15, 2018

(To be checked whether the schema.org tools would understand types if used that way.)

I just checked

{
"@context" : [
     "https://schema.org",
     {
       	"type" : "@type"
     }],
"type" : "Book",
"name" : "my title"
}

and the structured data testing tools processes it all right.

@mattgarrish
Copy link
Member

Does this also kill off the "infoset" eventually, or how does a canonical manifest co-exist with it if the canonical manifest is the result of following the infoset rules?

@iherman
Copy link
Member Author

iherman commented Aug 16, 2018

@mattgarrish good question... we will have to carefully formulate this but I do not think it kills off the infoset. Or, to put it another way, I am not sure it would kill off the infoset more than the JSON-LD manifest itself...

index.html Outdated
</ul>
is a single string or object <code>v</code>, then change the relevant term/value to<br><code>"term" : [v]</code>
</li>
<li>(<a href="#creators"></a>) if one of values <code>v</code> in the <code>manifest["term"]</code> array, where <code>term</code> is one of the <a href="#creators">creator</a> terms, is a single string, exchange that element of the array to<br><code>"term" : { "@type": "Person", "name" : [v]}</code></li>
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused by this step. Shouldn't this be:

"term": [{"@type": "Person", "name": "First"}] rather than "term": {"@type": "Person", "name": ["First"]}?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well... neither:-) Because this is step 4, everything is already an array, and this step refers to one of the elements of the array. In other words, it should rather be simply {"@type":"Person", "name":[v]} without the "term":

Will update this in a few.

Copy link
Member

Choose a reason for hiding this comment

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

Correct, that's step 4.

What's the point of step 5 then? Is this simply to add a default "type": "Person"? In this case, this should be applied when we have @value and @language as well (instead of a string).

But my other comment about Person/Organization in the WebIDL, applies here as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

No. It is to convert

"author": ["John Doe"]

to

"author": [{"@type": "Person", "name": ["John Doe"]}]

(The text conversion to objects are done in a later step.)

Copy link
Member

Choose a reason for hiding this comment

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

Right, that's what I meant as well.

But what about:

"author": [
  {
    "@value": "John Doe",
    "@language": "en"
  }
]

shouldn't this be converted to:

"author": [
  {
    "@type": "Person",
    "name": {
      "@value": "John Doe",
      "@language": "en"
    }
  }
]

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right, it should. I will have to rephrase it to say "string or object", or something like that. Will do.

index.html Outdated
is a single string or object <code>v</code>, then change the relevant term/value to<br><code>"term" : [v]</code>
</li>
<li>(<a href="#creators"></a>) if one of values <code>v</code> in the <code>manifest["term"]</code> array, where <code>term</code> is one of the <a href="#creators">creator</a> terms, is a single string, exchange that element of the array to<br><code>"term" : { "@type": "Person", "name" : [v]}</code></li>
<li>(<a href="#link-values"></a>) if one of values <code>v</code> in the <code>manifest["term"]</code> array, where <code>term</code> is one of the <a href="#resource-categorization-properties">resource categorization properties</a>, is a single string, exchange that element of the array to<br><code>"term" : { "@type": "PublicationLink", "url" : v}</code><br></li>
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if we really need to force a @type on every link. I don't think we'll keep that in our WebIDL.

Copy link
Member Author

Choose a reason for hiding this comment

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

The question what we do with @type when we map it against WebIDL. After all, we do have a PublicationLink interface, and the mapping of @type is, in some sense, mapping the object on the right interface in the WebIDL.

I would leave it as is for now, and come back to this when we discuss the details of the next step.

Copy link
Member

Choose a reason for hiding this comment

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

From a WebIDL perspective:

  • having a @type for each publication link is fairly useless (we know that we have a sequence of PublicationLink anyway)
  • we can't have a Person and an Organization dictionary if we plan on doing a straight conversion from JSON to WebIDL using the canonical manifest

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a 'canonical' conversion algorithm from JSON to WebIDL? Because if there is not, then we can define what happens with @type. We actually have to deal with the other @... terms as well.

Copy link
Member

Choose a reason for hiding this comment

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

This is the reference used in the Web App Manifest draft: https://heycam.github.io/webidl/#es-dictionary

Copy link
Member Author

Choose a reason for hiding this comment

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

@HadrienGardeur .

This is the reference used in the Web App Manifest draft: https://heycam.github.io/webidl/#es-dictionary

... which is, in fact, a strange reference because it describes an IDL->JavaScript mapping, not a JSON->IDL mapping that is used here. But o.k., it describes an equivalence of IDL and JS data, I guess it is acceptable.

I guess what we have to look at, in view of this mapping, is

  1. what we do with @type and @id, which are odd-man-out because they can be problematic as attribute names (I have not found a clear specification of the IDL term syntax, but I realized that the IDL parser working in respec simply swallows the @ sign when generating the final text.)
  2. What to do with the Person/Organization IDL interfaces.

To move ahead, maybe we should (after discussions today) agree to merge this PR with the canonical manifest as a concept and its current spec (ie, making it part of the current editor's draft) and then solve these two issues to update the draft. We can then look at the details of the conversion which, I believe, should simply list the type of error/warning checks that the processor should do no the final data. W.D.Y.T?

Copy link
Member

Choose a reason for hiding this comment

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

For Person/Organization, I'm not convinced that we need two separate IDL interfaces. A type for a universal interface would work as well since they'll contain the same terms anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

@HadrienGardeur, mainly if we follow this very close relationship between the canonical manifest and IDL, I agree with you. I would put 'type' required for contributors, and use that mechanism.

I think we should also map, via the context file, @value->@value, @language -> language, @id -> id, and @type -> type. We can do that, it seems that the google tools understand them, and it makes the mapping from the can. manifest to IDL easy and problem-less.

But we can do that only if we have a general WG agreement on the can. manifest, because it is a change affecting the whole manifest definition.

Copy link
Member

Choose a reason for hiding this comment

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

From an IDL standpoint, the type for Person/Organization isn't really required, as it won't make much of a difference for a UA if it's one or the other. Can't we default to Person?

Copy link
Member Author

Choose a reason for hiding this comment

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

The schema.org requirements means that some creators must be persons only, others can be either persons or organizations. If the typing is not there, schema.org consumers might reject properties (I think they do it already), meaning that, depending on the creators, these constraints must be checked.

- Added a rule for default order
- Changed the rules for links and creators to refer to single element in the array
- Minor formatting updates
The value `v` can also be a localizable string.
@iherman
Copy link
Member Author

iherman commented Aug 20, 2018

The proof-of-concept implementation has now a version that uses a separate manifest canonicalization step in line with what is in this PR:

iherman/WPManifest#2

@iherman iherman changed the title Towards and update of the lifecycle sections Introduction of a canonical manifest Aug 20, 2018
@HadrienGardeur
Copy link
Member

HadrienGardeur commented Aug 20, 2018

I've manually created an example of a canonical manifest based on the audiobook (Flatland) example: https://github.com/w3c/wpub/blob/master/experiments/audiobook/flatland-canonical.json

Is this correct @iherman?

Update: I just saw that I missed the name for each resource in the readingOrder, I'll update that and push it.

@iherman
Copy link
Member Author

iherman commented Aug 20, 2018

@HadrienGardeur at first glance yes, it is.

After the merge (tomorrow...) I will add a reference to it in the text. Thanks!

@iherman iherman merged commit 01bd2c8 into master Aug 21, 2018
@iherman iherman deleted the lifecycle-update branch August 21, 2018 03:55
@iherman
Copy link
Member Author

iherman commented Aug 21, 2018

Merged by virtue of the WG resolution

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