-
Notifications
You must be signed in to change notification settings - Fork 621
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
Separate NormalizedUnitSpec
from CompositeUnitSpec
#4440
Conversation
sounds good |
99e40ae
to
7e3df8e
Compare
f3e16ea
to
571dcef
Compare
@domoritz It looks like the schema generator does not generate the right output for |
Hmm, that's bad but I don't have time to look into it until next month. Can you try to create a small test case in the schema generator repo? |
FYI, this is extremely high priority as it's blocking several PRs downstream. |
Okay, I will try to make some time. Can you describe what exactly is wrong (besides "mark" and "encoding" missing?) |
It seems like Perhaps, the |
39e5019
to
20c838c
Compare
src/spec/unit.ts
Outdated
export type FacetedCompositeUnitSpec = GenericUnitSpec<EncodingWithFacet<string | RepeatRef>, AnyMark>; | ||
export type FacetedExtendedUnitSpec = ExtendedUnitSpec<FacetMapping<Field>>; | ||
|
||
// Note: The following three lines are equivalent to: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@domoritz I found a workaround, so you don't have to rush to fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. I should still look into it to make sure it’s not a problem elsewhere.
59045b6
to
d74ff73
Compare
build/vega-lite-schema.json
Outdated
} | ||
] | ||
}, | ||
"CompositeMarkDef": { | ||
"CompositeMarkUnitSpec<FacetMapping>": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<FacetMapping>
=> WithFacet
054753d
to
36da7aa
Compare
@@ -10644,6 +12026,55 @@ | |||
], | |||
"type": "object" | |||
}, | |||
"TopLevelCompositeMarkUnitSpec": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still incorrect due to the same problem...
export type CompositeMarkUnitSpec<
EE = {} // extra encoding parameter (for faceted composite unit spec)
> = ErrorBarUnitSpec<EE> | ErrorBandUnitSpec<EE> | BoxPlotUnitSpec<EE>;
… and separate NormalizedUnitSpec from CompositeUnitSpec
Closing in favor of #4528, which simplify this PR and avoids splitting unit specs into multiple pieces (for now). |
and introduce
ExtendedUnitSpec = NormalizedUnitSpec | CompositeUnitSpec
This makes the typing clearer between what's normalized (supported internally) vs what's composite unit spec only.
cc: @jakevdp
Merge #4493 First