@graph container support

[davidlehn]

Work-in-progress PR.

• Add "@container": "@graph" expand/compact tests.
• Add "@container": ["@graph", "@set"] expand/compact tests.
• Add spec text.
• Add support for additional @id or @index usage with @graph?

[gkellogg]

I'll get to my review this weekend, after completing my own implementation, thanks for doing the heavy lifting!

A concern is that, while this allows @graph containers, it doesn't really support named graphs, as the graphs created are anonymous with no possibility of reference, other than as the value of the property for which they're associated. Solving naming would requiring adding something like a @graphName keyword in the term definition, and could still be problematic if that name is a BNode. Probably best to address this as a simple issue, as this clearly solves an important use case, without having the ability to actually name the named graphs.

[davidlehn]

@gkellogg Comments on recent commits:

I think the @graphId part needs a spec example or two. At first glance of spec text and tests, I'm still not sure how it works. Keyword should also be added to the "Syntax Tokens and Keywords" section. Unfortunate to need a camel case keyword. It's the only one outside of framing spec? Potential issue is that cap-eye-dee (Id) and lower-el-dee (ld) look the same in some fonts (like right here as I type). I could see that causing confusion.

Removing the highlight bit is unfortunate. Would be nice to find a better solution for that.

You marked off the @index part as done but I think I was unclear on what I meant there. I was suggesting perhaps we want to allow combining @id and @index with @graph in @container. And just to be complete, allow @set in there too. Not sure if this would help with the @graphId bit too? So you could do things like:

{
  "@context": {
    "@vocab": "ex:",
    "index-container": {
      "@id": "ex:index-container",
      "@container": ["@graph", "@index"]
    },
    "id-container": {
      "@id": "ex:id-container",
      "@container": ["@graph", "@id"]
    }
  },
  "index-container": {
    "idx1": { ... }
  },
  "id-container": {
    "ima:id": { ... }
  }
}

Which would expand out into something like:

[
  {
    "ex:index-container": [
      {
        "@graph": ...,
        "@index": "idx1"
      }
    ],
    "ex:id-container": [
      {
        "@graph": ...,
        "@id": "ima:id"
      }
    ]
  }
]

[gkellogg]

I think the @GraphID part needs a spec example or two.

Sure

At first glance of spec text and tests, I'm still not sure how it works.

I'll, of course, add spec text; it's pretty straight-forward, but there may be some corner-cases to explore.

Keyword should also be added to the "Syntax Tokens and Keywords" section.

👍

Unfortunate to need a camel case keyword. It's the only one outside of framing spec? Potential issue is that cap-eye-dee (Id) and lower-el-dee (ld) look the same in some fonts (like right here as I type). I could see that causing confusion.

Open to suggestions, we could simply use @name.

Removing the highlight bit is unfortunate. Would be nice to find a better solution for that.

I've looked, and perhaps there's something hidden in highlight.js we could take advantage of. OTOH, it basically looks the same as it did in 1.0 (minus the diff-marking, which its just CSS).

You marked off the @Index part as done but I think I was unclear on what I meant there. I was suggesting perhaps we want to allow combining @id and @Index with @graph in @container.

What is done is that @container: @index can index graphs, but does not work with the @graph term logic. It's of dubious use, but was pretty simple. Absent use cases, maybe we should remove it and create issues to motivate other use cases.

[gkellogg]

@dlongley Thinking about it some more, it's probably better to use the "@container": ["@id", "@graph"] instead of introducing an @graphId (or perhaps @graphName keyword, which is not as flexible, as the name of the graph is fixed in the term definition. I'll need to spend some time on that, but (after making things consistent), I'm going to back out the @graphId changes and work on that.

[gkellogg]

@davidlehn and @dlongley Tests, syntax and algorithms are updated to reflect my implementation of basic graph containers, as well as the map versions. Please review.

[gkellogg]

I believe it's good to go now.

[gkellogg]

@dlongley We have support for "@graph", ["@graph", "@set"], ["@graph", "@index"], ["@graph", "@index", "@set"], ["@graph", "@id"], ["@graph", "@id", "@set"].

I'm not sure how useful ["@graph", "@index", "@set"] and ["@graph", "@id", "@set"], as, unless a document had multiple graphs with the same name, this would never happen. It does complicate the language a bit, so I'm inclined to remove support for that.

The only argument to leave it, is that @set is valid with all other combinations, other than @list.

[gkellogg]

Fixes #195. Fixes #481

[dlongley]

@gkellogg,

I'm not sure how useful ["@graph", "@Index", "@set"] and ["@graph", "@id", "@set"], as, unless a document had multiple graphs with the same name, this would never happen. It does complicate the language a bit, so I'm inclined to remove support for that.

The only argument to leave it, is that @set is valid with all other combinations, other than @list.

I think the ["@graph", "@index", "@set"] case doesn't require the same name for a graph... it could just could be a collection of unnamed graphs under a single index, right?

As for ["@graph", "@id", "@set"], if the only advantage is consistency for consistency's sake then it seems more compelling to remove support given that it complicates language and implementations with no real use cases. Of course, if supporting the @index combo version and not the @id combo version would make things even more complex, then it may as well stay. It would be good to get some more opinions though given that it looks like you've already put in the work to support both.

In any case, I'm +1 to merging the PR as is and happy defer any additional decisions to later/another PR. Great work, guys, I especially like that there was no need for an additional @graphId or similar keyword.

[dlongley]

LGTM, just needs rebasing due to conflicts.

[gkellogg]

As for ["@graph", "@id", "@set"], if the only advantage is consistency for consistency's sake then it seems more compelling to remove support given that it complicates language and implementations with no real use cases.

Looking at my implementation, I don't think it really makes implementation more complicated; one line in compaction, and a bit more complicated logic in error detection when looking at the term definition. The language in Syntax is slightly more complicated, but probably not worth changing now.

[davidlehn]

@gkellogg Was this supposed to be removed?

[gkellogg]

No, this remains, and is what is used by verifiable credentials. We may need to add more about graph id and index containers, though.

What we removed was anything about graphId.