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

3.0.0-rc0: strange schema for FoldTransform.as #4105

Closed
jakevdp opened this issue Aug 1, 2018 · 12 comments
Closed

3.0.0-rc0: strange schema for FoldTransform.as #4105

jakevdp opened this issue Aug 1, 2018 · 12 comments

Comments

@jakevdp
Copy link
Contributor

jakevdp commented Aug 1, 2018

The schema definition for FoldTransform.as is a bit strange. It's supposed to be a two-element array, correct? So what is the purpose of additionalItems: anyOf: ["string", "string"]?

I'm wondering if this is indicative of some mis-specification in the typescript.

@jakevdp
Copy link
Contributor Author

jakevdp commented Aug 1, 2018

Also, I'm not certain what vega-lite version the currently-live docs refer to, but the described syntax of the fold transform there does not match the schema: https://vega.github.io/vega/docs/transforms/fold/

@domoritz
Copy link
Member

domoritz commented Aug 1, 2018

Here is the type in Typescript: https://github.com/vega/vega-typings/blob/e22ed3faa7dc7e9bba5607c249f526f98de23436/types/spec/transform.d.ts#L186

The type is a bit odd but not wrong. It says that we want an array with at least two items and the items should be strings. Any additional item should also be a string.

We can try to make the schema more strict.

@jakevdp
Copy link
Contributor Author

jakevdp commented Aug 1, 2018

Agree that it's technically correct but a bit odd.

What caught me off-guard is the use of an array of schemas for items. It's currently the only place in the vega-lite schema that that is used, and the fact that it led to such a strange schema made me wonder if the schema building tool was not handling it properly.

@jakevdp
Copy link
Contributor Author

jakevdp commented Aug 1, 2018

Maybe changing as?: [string, string]; to as?: string[]; would give a cleaner result?

Edit: or perhaps

  /**
   * An array of allowable step sizes to choose from.
   * @minItems 1
   */
  as?: string[]

This seems to work properly elsewhere in the typing specification.

@domoritz
Copy link
Member

domoritz commented Aug 1, 2018

I'll look into it over at vega/ts-json-schema-generator#40.

@domoritz
Copy link
Member

domoritz commented Aug 1, 2018

The typings now come from Vega-Typings and it would be nice to keep the strict TS typings.

@kanitw
Copy link
Member

kanitw commented Aug 1, 2018

It says that we want an array with at least two items and the items should be strings. Any additional item should also be a string.

This is a bit inaccurate. The type there means there should b _strictly two items, and each of them should be a string.

@domoritz
Copy link
Member

domoritz commented Aug 1, 2018

This is a bit inaccurate. The type there means there should b _strictly two items, and each of them should be a string.

The schema type is less strict and just says what I wrote. Your description is for the TS type.

@kanitw
Copy link
Member

kanitw commented Aug 1, 2018

I see. Sounds like a bug that additionalItem is included.

@domoritz
Copy link
Member

domoritz commented Aug 1, 2018

It's not wrong, just imprecise.

domoritz added a commit that referenced this issue Aug 2, 2018
@domoritz
Copy link
Member

domoritz commented Aug 2, 2018

Fixed in #4107

@domoritz domoritz closed this as completed Aug 2, 2018
@jakevdp
Copy link
Contributor Author

jakevdp commented Aug 2, 2018

Thanks!

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

No branches or pull requests

3 participants