Support `field` in condition #1980

kanitw · 2017-02-24T22:59:09Z

Follow up from #1971.

It's probably more natural to do the following way in many cases:

x: {
  condition: { selection: <name>,  field: ..., type: ...},
  value: ...
}

Currently this PR only contains interface changes, but haven't make it work.

This will require a major refactor since we can no longer just refer to
encoding.<channel>.field for accessing field, so I'm gonna punt on it for now.

- Conditional as a generic type for conditional channelDef - Condition as a generic type of channelDef with `selection` predicate

…of encoding.ts

…annelDef.condition)` if applicable - Introduce - `getFieldDef()` which returns the fieldDef -- either from the outer channelDef or from the condition of channelDef - `hasConditionFieldDef()` and `isConditionalDef()`

- Correct type signature for subsets of Channel (SingleDefChannel, ScaleChannel, etc.) - Remove outdated logic for row/column's axis (we no longer use axis for row/column -- and use header instead!) - Make model.fieldDef() references check for undefined

…n schema`

arvind

Hurrah 🎉 for this PR! The changes look good to me. I've left minor questions/comments throughout that may yield some cleaner code paths.

arvind · 2017-06-09T18:56:38Z

src/fielddef.ts

+ */
+export function hasConditionFieldDef<F>(channelDef: ChannelDef<F>): channelDef is (ValueDef<any> & {condition: FieldDef<F>}) {
+  return !!channelDef && !!channelDef.condition && isFieldDef(channelDef.condition);
+}

 export function isFieldDef<F>(channelDef: ChannelDef<F>): channelDef is FieldDef<F> | PositionFieldDef<F> | LegendFieldDef<F, any> | OrderFieldDef<F> | TextFieldDef<F> {
  return !!channelDef && (!!channelDef['field'] || channelDef['aggregate'] === 'count');


getFieldDef below looks good, but I'm curious why isFieldDef doesn't account for a Conditional field def?

Relatedly, I see that several isFieldDef calls have been replaced with getFieldDef to account for the conditional. If there's a difference in how the two should be used/what they should check for, perhaps we can name them more clearly to reflect that? Unfortunately I don't have suggestions yet because I don't grasp the difference, sorry!

Yeah, say a channelDef could be

FieldDef (with no condition)

FieldDef with ConditionValueDef

(1 & 2 are ConditionalFieldDef as I make condition? so I don't have to declare too many types)

ValueDef (with no condition)

ValueDef with ConditionFieldDef

ValueDef with ConditionValueDef

(3-5 are ConditionalValueDef as I make condition? so I don't have to declare too many types)

ConditionFieldDef only (akin to if without else)

ConditionValueDef only

(6-7 are ConditionOnlyDef)

isFieldDef is more scoped to one object and can check if the object is a fieldDef.
The input can be either:

a) the parent object of a channelDef (encoding.<channel>), which will return true for 1 or 2,

b) the condition object of a channelDef (encoding.<channel>.condition), which will return true for case 4 or 6.

On the other hand, getFieldDef is for getting the fieldDef from a channelDef.
Given a channelDef (from encoding.<channel>), this will return either the parent channelDef object (for case 1 or 2) or its condition object (for case 4 or 6).

In a way, getFieldDef is more like getFieldDefFromChannelDef but it is kinda verbose, so I just chose the brief getFieldDef. That said, I agree that it can be confusing.

If you have better name suggestions, let me know :)

arvind · 2017-06-09T18:57:20Z

build/vega-lite-schema.json

@@ -1032,32 +1032,47 @@
      "$ref": "#/definitions/CompositeUnitSpecAlias",
      "description": "Unit spec that can have a composite mark."
    },
-    "Condition<(string|number)>": {
-      "additionalProperties": false,
+    "Condition<(LegendFieldDef<Field,number>|ValueDef<number>)>": {


Reminder to update rename-schema for these keys.

arvind · 2017-06-09T19:01:09Z

src/compile/unit.ts

@@ -156,10 +156,13 @@ export class UnitModel extends ModelWithField {
      if (isFieldDef(channelDef)) {
        fieldDef = channelDef;
        specifiedScale = channelDef.scale;
+      } else if (isConditionalDef(channelDef) && isFieldDef(channelDef.condition)) {
+        fieldDef = channelDef.condition;
+        specifiedScale = channelDef.condition.scale;


Could these first two branches be consolidated using the new getFieldDef method?

Yeah I tried this earlier too. However, TypeScript won't know that channelDef.condition is not only a FieldDef, but also a ScaleFieldDef. So we either have to do type casting or have to do this and I generally prefer to avoid casting.

arvind · 2017-06-09T19:02:38Z

src/compile/unit.ts

-          _legend[channel] = {...legendSpec};
+      if (channelDef) {
+        const legend = isFieldDef(channelDef) ? channelDef.legend :
+          (channelDef.condition && isFieldDef(channelDef.condition)) ? channelDef.condition.legend : null;


Similarly, here, rather than multiple ternaries depending on where the fieldDef might be, could this be simplified with a single getFieldDef call?

Similar to #1980 (comment), TS won't know that this is a LegendFieldDef, not just FieldDef.

arvind · 2017-06-09T19:13:17Z

src/channel.ts

@@ -55,6 +55,10 @@ export const TOOLTIP = Channel.TOOLTIP;
 export const CHANNELS = [X, Y, X2, Y2, ROW, COLUMN, SIZE, SHAPE, COLOR, ORDER, OPACITY, TEXT, DETAIL, TOOLTIP];
 const CHANNEL_INDEX = toSet(CHANNELS);

+export const SINGLE_DEF_CHANNELS = [X, Y, X2, Y2, ROW, COLUMN, SIZE, SHAPE, COLOR, OPACITY, TEXT, TOOLTIP];


What is a "single def" channel? Perhaps a comment to explain what this class of channels is and why it's needed?

Good catch.

I'll add

/** * Channels cannot have an array of channelDef. * model.fieldDef, getFieldDef only work for these channels. * * (The only two channels that can have an array of channelDefs are "detail" and "order". * Since there can be multiple fieldDefs for detail and order, getFieldDef/model.fieldDef * are not applicable for them. Similarly, selection projecttion won't work with "detail" and "order".) */

arvind · 2017-06-09T19:15:47Z

src/compile/axis/rules.ts

-    return false;
-  }
-
+export function grid(model: UnitModel, channel: SpatialScaleChannel, isGridAxis: boolean) {


For my edification, why do we no longer skip grid for ROW and CHANNEL?

With the new layout operator, we no longer use axes for row/column but use layout header instead. Here I basically forgot to remove the code.

arvind · 2017-06-09T19:17:27Z

src/compile/selection/multi.ts

      // Binned fields should capture extents, for a range test against the raw field.
-      return model.fieldDef(channel).bin ? (bins[p.field] = 1,
+      // FIXME: Arvind -- please log proper warning when the specified encoding channel has no field


Good catch!

kanitw · 2017-06-10T02:50:18Z

build/vega-lite-schema.json

+      ],
+      "type": "object"
+    },
+    "Condition<(LegendFieldDef|ValueDef<string>)>": {


@domoritz looks like we have a bug in the schema generator?

Some properties are missing here

(None of the properties from LegendFieldDef or ValueDef are here at all.)

@domoritz

1) Avoid schema generator bug (cc: @domoritz) 2) Produce better schema Also update rename script

kanitw · 2017-06-11T03:05:21Z

@domoritz

Upgrade to https://github.com/vega/typescript-to-json-schema/releases/tag/v0.4.0

I got Error: Cannot find module '../dist/typescript-to-json-schema' after upgrading

kanitw · 2017-06-11T03:07:12Z

@arvind -- I added a few more commits to simplify the schema.
Now we longer have generics in the schema. I also re-declare the part that triggers a bug in the schema generator with a better way to define it and also avoid the bug.

arvind · 2017-06-11T03:50:13Z

Travis is detecting differences with the schema. Is that due to the issue you mention above?

kanitw · 2017-06-11T03:56:53Z

@arvind I don't think so. I just force push an update. looks like I somehow forgot to commit the diff.

kanitw · 2017-06-11T03:57:45Z

Note that we can update the schema and remove b82b8ab in a separate PR.
So if this passes and you like everything else, we can merge.

vega deleted a comment from domoritz Jun 8, 2017

kanitw added 2 commits June 8, 2017 16:09

Declare Conditional and Condition

57d8176

- Conditional as a generic type for conditional channelDef - Condition as a generic type of channelDef with `selection` predicate

Declare ConditionLegend/TextDef, PositionDef to simplify declaration …

e39ec50

…of encoding.ts

kanitw force-pushed the kw/condition-field branch 6 times, most recently from 45fcf6b to 4fe69f8 Compare June 9, 2017 06:05

kanitw force-pushed the kw/condition-field branch from 4fe69f8 to 323e4bb Compare June 9, 2017 06:08

kanitw mentioned this pull request Jun 9, 2017

Is encodings in BaseSelectionDef only channel #2483

Closed

kanitw force-pushed the kw/condition-field branch 2 times, most recently from ff059d7 to a789df8 Compare June 9, 2017 18:25

kanitw added 7 commits June 9, 2017 11:35

Add missing ConditionOnlyDef

5c873ef

Make wrapCondition support ConditionFieldDef

9a953fa

Remove unused model.channels()

46fbafe

Manually declare SingleDefChannel as this leads to error with `npm ru…

b82b8ab

…n schema`

Remove "not" from example specs

902dbb9

Add test for aggregating ConditionField

584361b

kanitw force-pushed the kw/condition-field branch from a789df8 to 584361b Compare June 9, 2017 18:36

kanitw changed the title ~~[WIP] Support field in condition~~ Support field in condition Jun 9, 2017

kanitw requested a review from arvind June 9, 2017 18:36

kanitw mentioned this pull request Jun 9, 2017

This generator does not support type generate of typeof array item vega/ts-json-schema-generator#1

Closed

arvind approved these changes Jun 9, 2017

View reviewed changes

vega deleted a comment from arvind Jun 9, 2017

kanitw commented Jun 10, 2017

View reviewed changes

kanitw added 2 commits June 10, 2017 10:57

Add comment

49a9688

Simplify schema

ddf2317

kanitw force-pushed the kw/condition-field branch from d3e0e21 to ddf2317 Compare June 10, 2017 17:57

Remove ConditionLegend/TextDef, PositionDef to simplify schema file

61a62d7

kanitw force-pushed the kw/condition-field branch from 5311e14 to 61a62d7 Compare June 11, 2017 02:26

kanitw added 2 commits June 10, 2017 19:48

Separate Condition<F | V> to Condition<F> | Condition<V> to

06cc8dc

1) Avoid schema generator bug (cc: @domoritz) 2) Produce better schema Also update rename script

Further remove generic for Conditon*Def<...> and TopLevel<...>

28a18bc

kanitw force-pushed the kw/condition-field branch from 0e5925f to 28a18bc Compare June 11, 2017 03:03

kanitw mentioned this pull request Jun 11, 2017

Incorrect output for Generic with UnionType vega/ts-json-schema-generator#3

Closed

Fix typo in the rename script

012807d

kanitw force-pushed the kw/condition-field branch from ba37042 to 012807d Compare June 11, 2017 03:53

arvind merged commit b9b2830 into master Jun 11, 2017

arvind deleted the kw/condition-field branch June 11, 2017 04:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `field` in condition #1980

Support `field` in condition #1980

kanitw commented Feb 24, 2017 •

edited

arvind left a comment

arvind Jun 9, 2017

arvind Jun 9, 2017

kanitw Jun 9, 2017

arvind Jun 9, 2017

arvind Jun 9, 2017

kanitw Jun 9, 2017 •

edited

arvind Jun 9, 2017

kanitw Jun 9, 2017

arvind Jun 9, 2017

kanitw Jun 9, 2017

arvind Jun 9, 2017

kanitw Jun 9, 2017

arvind Jun 9, 2017

kanitw Jun 10, 2017

kanitw Jun 10, 2017

kanitw commented Jun 11, 2017

kanitw commented Jun 11, 2017

arvind commented Jun 11, 2017

kanitw commented Jun 11, 2017

kanitw commented Jun 11, 2017

Support field in condition #1980

Support field in condition #1980

Conversation

kanitw commented Feb 24, 2017 • edited

arvind 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

kanitw Jun 9, 2017 • edited

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

Choose a reason for hiding this comment

kanitw commented Jun 11, 2017

kanitw commented Jun 11, 2017

arvind commented Jun 11, 2017

kanitw commented Jun 11, 2017

kanitw commented Jun 11, 2017

Support `field` in condition #1980

Support `field` in condition #1980

kanitw commented Feb 24, 2017 •

edited

kanitw Jun 9, 2017 •

edited