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

Cleaner schema #3186

Merged
merged 1 commit into from
Nov 6, 2017
Merged

Cleaner schema #3186

merged 1 commit into from
Nov 6, 2017

Conversation

domoritz
Copy link
Member

@domoritz domoritz commented Nov 3, 2017

Needs vega/ts-json-schema-generator#19

@jakevdp, before I merge this, can you check that this schema in this PR works for you and doesn't have unused definitions.

@jakevdp
Copy link
Contributor

jakevdp commented Nov 3, 2017

This got most of them, but there are still a handful of unneeded definitions:

{'BoxPlotConfig',
 'BoxPlotConfigMixins',
 'DataFormatType',
 'FacetedCompositeUnitSpecAlias'}

@domoritz
Copy link
Member Author

domoritz commented Nov 3, 2017

Hmm, the boxplot defs are there because we use @hide. I have to check where FacetedCompositeUnitSpecAlias comes from.

@domoritz
Copy link
Member Author

domoritz commented Nov 3, 2017

DataFormatType is subtle. The issue is that we are extending an abstract interface but in the extensions, we override the type. I figured out a way to remove it, though.

@domoritz
Copy link
Member Author

domoritz commented Nov 3, 2017

There is a bug in the schema generator that causes some other classes to be included.

external.ts

export interface CC {
    c: number;
}

export interface C extends CC {
}

// or: export type C = CC;

test.ts

import { C } from "./external";

export interface MyObject extends C {
    foo: number;
}

@domoritz
Copy link
Member Author

domoritz commented Nov 4, 2017

I figured out everything except for FacetedCompositeUnitSpecAlias.

@domoritz domoritz force-pushed the dom/generate-clean branch 2 times, most recently from d9519fb to 6c177aa Compare November 4, 2017 00:20
@domoritz domoritz changed the title [WIP] cleaner schema Cleaner schema Nov 4, 2017
@domoritz domoritz requested a review from kanitw November 4, 2017 00:21
Copy link
Member

@kanitw kanitw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need input from @arvind about selection test changes

* The default format type is determined by the extension of the file URL.
* If no extension is detected, `"json"` will be used by default.
*/
type?: DataFormatType;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is type removed from here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure that the type displayed in the table in data.md is correct.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we override it in all cases anyway. I fixed the table in data.md.

@@ -21,7 +21,7 @@ function getModel(markType: any) {
model.component.selection = selection.parseUnitSelection(model, {
"one": {"type": "single", "nearest": true},
"two": {"type": "multi", "nearest": true},
"three": {"type": "interval", "nearest": true},
"three": {"type": "interval"},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these get removed? We will need input from @arvind if removing these make sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m guessing these were removed because they are actually not valid Vega-Lite statements. I just had them here to verify that we did the right thing (eg ignore or throw errors as appropriate) if a user specified them. Will go through the other selection test changes too and comment if something looks amiss.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And now the typescript compiler is smart enough to notice.

Copy link
Member

@arvind arvind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes to selection tests LGTM.

@domoritz
Copy link
Member Author

domoritz commented Nov 5, 2017

@jakevdp Let us know if the schema is clean now. @kanitw feel free to merge when you think it's ready.

@kanitw
Copy link
Member

kanitw commented Nov 6, 2017

Do you want to wait for @jakevdp's input? If not, feel free to merge.

@domoritz
Copy link
Member Author

domoritz commented Nov 6, 2017

The schema is clean except for FacetedCompositeUnitSpecAlias but I can't figure out where that's coming from right now.

@domoritz domoritz merged commit 41bdc37 into master Nov 6, 2017
@domoritz domoritz deleted the dom/generate-clean branch November 6, 2017 03:26
@jakevdp
Copy link
Contributor

jakevdp commented Nov 6, 2017

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

Successfully merging this pull request may close these issues.

4 participants