Add Invalid Encoding Channel Warning #3426

invokesus · 2018-02-28T14:32:59Z

Please:

Make your pull request atomic, fixing one issue at a time unless there are many relevant issues that cannot be decoupled.
Provide a test case & update the documentation under site/docs/
Make lint and test pass. (Run yarn lint and yarn test. If your change affects Vega outputs of some examples, please run yarn build:example EXAMPLE_NAME to re-compile a specific example or yarn build:examples to re-compile all examples.)
Make sure you have rebased onto the master branch.
Provide a concise title so that we can just copy it to our release note.
- Use imperative mood and present tense.
- Mention relevant issues. (e.g., #1)

domoritz · 2018-02-28T17:50:51Z

Thank you for the pull request.

Please make this a warning and delete the encoding instead of throwing an error. Also, let's add a test.

invokesus · 2018-03-01T18:24:32Z

The warning and deletion of encoding is already implemented in encoding.ts.
I was thinking of adding a case for unrecognized Channel here. So that, getSupportedMark returns an empty dictionary, causing supportMark to return false and finally the warning being triggered in normalizeEncoding.
Is this in the right direction?

domoritz · 2018-03-01T19:41:44Z

I'd trace what causes the error in #3415 and make sure that it's handled either by the code you found or something new you write.

invokesus · 2018-03-01T23:15:20Z

Updated the PR. 🙂

kanitw

Thank you so much for submitting a PR.

Please see my comments. I think you're getting there. :)

kanitw · 2018-03-03T17:08:34Z

src/channel.ts

@@ -214,6 +214,8 @@ export function getSupportedMark(channel: Channel): SupportedMark {
      return {point: true, geoshape: true};
    case TEXT:
      return {text: true};
+    default:


I wonder if this is the right place to correct this?

If the channel you have isn't a valid Channel at all, then it doesn't satisfy the signature of this method.

A better check would be using isChannel method to check if a key of the encoding object is a channel.

Made the changes. Thanks for the help!

kanitw · 2018-03-03T17:08:51Z

test/compile/unit.test.ts

+            _y: {type: 'quantitative'}
+          }
+        } as any); // To make parseUnitModel accept the model with invalid encoding channel
+        assert.equal(model.encoding.shape, undefined);


I'm not sure why this is related to model.encoding.shape.

Removed the assert.

kanitw

Thanks. A few more minor correction and this should be good to go! :)

kanitw · 2018-03-04T00:25:16Z

src/log.ts

@@ -175,6 +175,10 @@ export namespace message {
    return `${channel} dropped as it is incompatible with "${markOrFacet}"${when ? ` when ${when}` : ''}.`;
  }

+  export function invalidEncodingChannel(encoding: string) {


encoding normally refer to encoding mapping, which is an object.

Rename to channel would be less misleading.

kanitw · 2018-03-04T00:25:57Z

src/log.ts

@@ -175,6 +175,10 @@ export namespace message {
    return `${channel} dropped as it is incompatible with "${markOrFacet}"${when ? ` when ${when}` : ''}.`;
  }

+  export function invalidEncodingChannel(encoding: string) {
+    return `${encoding} dropped as it is invalid.`;


${channel}-encoding is dropped as ${channel} is not a valid encoding channel.

kanitw · 2018-03-04T00:26:29Z

test/compile/unit.test.ts

@@ -19,6 +19,17 @@ describe('UnitModel', function() {
        assert.equal(localLogger.warns[0], log.message.incompatibleChannel(SHAPE, BAR));
      }));

+    it('should drop invalid channel and throws warning', log.wrap((localLogger) => {
+        const INVALID_CHANNEL = '_y';


remove this line and just use "_y" below?

domoritz · 2018-03-04T01:15:26Z

Thank you @invokesus!

invokesus force-pushed the master branch 2 times, most recently from ec5d71b to e91c4fe Compare March 1, 2018 22:57

invokesus changed the title ~~Add Invalid Encoding Channel Error Message~~ Add Invalid Encoding Channel Warning Mar 1, 2018

kanitw requested changes Mar 3, 2018

View reviewed changes

invokesus force-pushed the master branch 2 times, most recently from b53d1ab to 13e46a8 Compare March 3, 2018 19:18

kanitw mentioned this pull request Mar 4, 2018

Invalid encoding channel should fail more gracefully #3415

Closed

kanitw requested changes Mar 4, 2018

View reviewed changes

Add Invalid Encoding Channel Warning

cc2594c

invokesus force-pushed the master branch from 13e46a8 to cc2594c Compare March 4, 2018 00:38

kanitw approved these changes Mar 4, 2018

View reviewed changes

kanitw merged commit 3f66a77 into vega:master Mar 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Invalid Encoding Channel Warning #3426

Add Invalid Encoding Channel Warning #3426

invokesus commented Feb 28, 2018 •

edited

Loading

domoritz commented Feb 28, 2018

invokesus commented Mar 1, 2018

domoritz commented Mar 1, 2018

invokesus commented Mar 1, 2018

kanitw left a comment

kanitw Mar 3, 2018

invokesus Mar 3, 2018

kanitw Mar 3, 2018

invokesus Mar 3, 2018

kanitw left a comment •

edited

Loading

kanitw Mar 4, 2018

invokesus Mar 4, 2018

kanitw Mar 4, 2018

kanitw Mar 4, 2018

domoritz commented Mar 4, 2018

Add Invalid Encoding Channel Warning #3426

Add Invalid Encoding Channel Warning #3426

Conversation

invokesus commented Feb 28, 2018 • edited Loading

domoritz commented Feb 28, 2018

invokesus commented Mar 1, 2018

domoritz commented Mar 1, 2018

invokesus commented Mar 1, 2018

kanitw 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

kanitw left a comment • edited Loading

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

domoritz commented Mar 4, 2018

invokesus commented Feb 28, 2018 •

edited

Loading

kanitw left a comment •

edited

Loading