Graph id type maps #212

gkellogg · 2018-01-12T00:56:12Z

This adds support for graph indexing on @id and @index, as well as id maps and type maps.

It is based on the processingMode branch, but is targeted for 0.6.x. The 0.6.x branch will require rebasing from 0.5.x until that branch is completed.

davidlehn · 2018-01-12T02:41:41Z

lib/graphTypes.js

+  // 3. It may have '@id' or '@index'
+  return types.isObject(v) &&
+    '@graph' in v &&
+    Object.keys(v).filter(key => key != '@id' && key != '@index').length === 1;


davidlehn · 2018-01-12T02:51:19Z

lib/expand.js

+    // and value is not, itself, a graph
+    // index cases handled above
+    if(container.includes('@graph') &&
+      !container.some(key => key == '@id' || key == '@index') &&


dlongley

Style nits mostly. Then LGTM.

dlongley · 2018-01-18T16:07:34Z

lib/compact.js

+            // add compactedItem to map, using value of `@index` or `@none`
+            _addValue(
+              mapObject, (expandedItem['@index'] || '@none'), compactedItem,
+              {propertyIsArray: (!options.compactArrays || container.includes('@set'))});


The code in these first two cases is really similar and could potentially be combined -- but that can wait until we do some clean up later. Just making a note of it.

This better?

I meant the code is the same for the cases where @index and where @id are present other than the second parameter to _addValue.

dlongley · 2018-01-18T16:08:43Z

lib/compact.js

          // get or create the map object
-          let mapObject;
+          let mapObject, key;


Declare vars on new lines:

let mapObject; let key;

But the key declaration should really be moved down to where it's immediately relevant, so right above its first use.

dlongley · 2018-01-18T16:14:45Z

lib/compact.js

+            delete compactedItem[idKey];
+          } else if(container.includes('@type')) {
+            const typeKey = api.compactIri({activeCtx, iri: '@type', vocab: true});
+            var types;


Use let types, not var.

dlongley · 2018-01-18T16:17:58Z

lib/compact.js

+            switch(types.length) {
+            case 0: delete compactedItem[typeKey]; break;
+            case 1: compactedItem[typeKey] = types[0]; break;
+            default: compactedItem[typeKey] = types; break;


Use line breaks:

switch(types.length) { case 0: delete compactedItem[typeKey]; break; case 1: compactedItem[typeKey] = types[0]; break; default: compactedItem[typeKey] = types; break; }

Or shorten to if/else:

if(types.length === 0) { delete compactedItem[typeKey]; } else if(types.length === 1) { compactedItem[typeKey] = types[0]; } else { compactedItem[typeKey] = types; }

dlongley · 2018-01-18T16:19:46Z

lib/compact.js

          }

          // add compact value to map object using key from expanded value
          // based on the container type
-          const c = container.includes('@language') ? '@language' : '@index';
-          _addValue(mapObject, expandedItem[c], compactedItem);
+          _addValue(mapObject, key, compactedItem,


Our coding style rule for long parameter lists is:

When moving any function parameters down a line, move them all down a line (and then add new lines as needed to fit within 80-chars, but do not leave any on the same line as the function as it improves readability):

_addValue( mapObject, key, compactedItem, {propertyIsArray: container.includes('@set')});

dlongley · 2018-01-18T16:20:43Z

lib/context.js

+            {code: 'invalid container mapping', context: localCtx});
+        }
+      } else if(container.includes('@graph')) {
+        if(container.some(key => key != '@graph' && key != '@id' && key != '@index' && key != '@set')) {


Use triple equals.

gkellogg · 2018-01-18T23:41:56Z

I made the requested changes. Note that this will need a rebase to fit with the other two PRs.

davidlehn · 2018-01-19T01:07:05Z

lib/compact.js

+    } else if(_isObject(value) && !_isValue(value)) {
+      containers.push('@id@set');
+      containers.push('@id');
+      containers.push('@type@set');


Flipped order?

Should be @set@type, and can push can take multiple arguments.

Based on json-ld/json-ld.org#569 it's even more complicated, if we want to prioritize order based on the actual value, and allow @none to be used. The work's not all done fo this yet, but may as well get this in order.

More container checking for specifics of `@list` and `@graph`. Don't expand values of graph container properties which are already graphs.

- adds a BNode issuer to compact to generate identifiers, if necessary.

… graphs.

…m013).

…et` in `@container`.

gkellogg requested review from davidlehn and dlongley January 12, 2018 00:56

davidlehn reviewed Jan 12, 2018

View reviewed changes

gkellogg force-pushed the graph-id-type-maps branch from 51cf8fb to 22ec6b1 Compare January 12, 2018 22:06

gkellogg force-pushed the 0.6.x branch from 6cd4fae to ad0740a Compare January 12, 2018 22:06

gkellogg force-pushed the graph-id-type-maps branch from 22ec6b1 to 00e321c Compare January 12, 2018 23:55

gkellogg added this to the v0.6.x milestone Jan 13, 2018

dlongley requested changes Jan 18, 2018

View reviewed changes

dlongley approved these changes Jan 18, 2018

View reviewed changes

davidlehn reviewed Jan 19, 2018

View reviewed changes

gkellogg added 12 commits January 18, 2018 20:06

Added isGraph in addition to isSimpleGraph.

f71eab3

More container checking for specifics of `@list` and `@graph`. Don't expand values of graph container properties which are already graphs.

Update compaction to check for graphs, rather than just simple graphs.

92ebea0

Expand container with @graph and @index.

21bcd3f

Expand @id index with @graph container.

ee93f33

Compact graphs with @container: @graph and @id or @index.

9eee397

- adds a BNode issuer to compact to generate identifiers, if necessary.

Support @id and @type map expansion.

2b9b55b

Compact using @id and @type maps.

268a5f8

Added CHANGELOG additions for @id, @type maps, along with indexed…

4b49583

… graphs.

Old habits die hard: !== instead of !=.

11f7478

Old habbits die hard: !== instead of !=.

fbd959b

Stylistic improvements.

3d550c8

Update container prioritization in compactIri.

5bb6924

gkellogg force-pushed the graph-id-type-maps branch from ee9a13b to 5bb6924 Compare January 19, 2018 04:10

gkellogg added 2 commits January 20, 2018 12:27

For type maps, use a context defined for that particular key (expand#…

efd7a15

…m013).

Include Compact #ts tests which look for combinations including `@s…

221487b

…et` in `@container`.

gkellogg merged commit b7318d7 into 0.6.x Jan 20, 2018

gkellogg deleted the graph-id-type-maps branch January 20, 2018 20:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Graph id type maps #212

Graph id type maps #212

gkellogg commented Jan 12, 2018

davidlehn Jan 12, 2018

davidlehn Jan 12, 2018

dlongley left a comment

dlongley Jan 18, 2018

gkellogg Jan 19, 2018

dlongley Jan 19, 2018

dlongley Jan 18, 2018

dlongley Jan 18, 2018

dlongley Jan 18, 2018

dlongley Jan 18, 2018

dlongley Jan 18, 2018

gkellogg commented Jan 18, 2018

davidlehn Jan 19, 2018

gkellogg Jan 19, 2018

Graph id type maps #212

Graph id type maps #212

Conversation

gkellogg commented Jan 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlongley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkellogg commented Jan 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment