Add Fold Transform #3813
Add Fold Transform #3813
Conversation
src/compile/data/fold.ts
Outdated
|
|
||
| public assemble(): VgFoldTransform { | ||
| const fields: string[] = []; | ||
| for (const field of this.transform.fold) { |
There was a problem hiding this comment.
Why do you have to convert undefined to null? Can't you just do fields: this.transform.fold?
src/compile/data/fold.ts
Outdated
| } | ||
| } | ||
|
|
||
| private getDefaultName(): [string, string] | undefined { |
There was a problem hiding this comment.
This method do more than returning default names. (It returns non-default specified value too, so the method name is a bit misleading.)
| const fold = head = new FoldTransformNode(head, t); | ||
|
|
||
| for (const field of keys(fold.producedFields())) { | ||
| ancestorParse.set(field, 'derived', false); |
There was a problem hiding this comment.
Don't forget to refactor these once we merge all of fold and flatten PR.
| @@ -1,4 +1,5 @@ | |||
| import {AggregateOp} from 'vega'; | |||
| import {FoldTransform as VgFoldTransform} from 'vega-typings'; | |||
There was a problem hiding this comment.
Great! This is exactly what we need.
@domoritz I wonder if we should import and reexport from this file so the rest of the codebase do not have to do as and thus will have consistent name?
src/compile/data/fold.ts
Outdated
|
|
||
| private getDefaultName(): [string, string] | undefined { | ||
| const as = this.transform.as; | ||
| if (as && isString(as[0]) && isString(as[1])) { |
There was a problem hiding this comment.
-
I don't think you need to check if each item is a string as that's already enforced by the schema. Just need to check if they exist.
-
if as[0] is specified, but as[1] isn't. Shouldn't we use
[as[0], 'value']?
test/compile/data/fold.test.ts
Outdated
| as: undefined | ||
| }); | ||
| }); | ||
| it ('should return proper produced fields for no "as"', () => { |
There was a problem hiding this comment.
Please add line breaks between tests.
test/compile/data/fold.test.ts
Outdated
| import {Transform} from '../../../src/transform'; | ||
|
|
||
| describe('compile/data/fold', () => { | ||
| describe('Testing FoldTransformNode', () => { |
There was a problem hiding this comment.
'Testing FoldTransformNode' => 'FoldTransformNode'
Everything under test is a test. :)
src/transform.ts
Outdated
|
|
||
| /** | ||
| * The output field names for the key and value properties produced by the fold transform. | ||
| * The default is ["key", "value"]. |
There was a problem hiding this comment.
Always use __Default value:__ and wrap the default value in back ticks
__Default value:__ `["key", "value"]`
behavior for single element 'as' array, rename getDefaultNames => getNames.
src/compile/data/fold.ts
Outdated
| } | ||
|
|
||
| public producedFields() { | ||
| const names = this.getNames(); |
There was a problem hiding this comment.
It's weird that you're calling getNames() again in producedFields().
The whole point of setting as in constructor is that you can just simply uses as to determine producedFields.
test/compile/data/fold.test.ts
Outdated
| assert.deepEqual(fold.producedFields(), {'key': true, 'value': true}); | ||
| }); | ||
|
|
||
| it ('should return proper produced fields for no "as"', () => { |
There was a problem hiding this comment.
Forgot to change this. Should be complete instead of no.
There was a problem hiding this comment.
Can you fix it and make test pass
src/compile/data/fold.ts
Outdated
| @@ -25,17 +25,6 @@ export class FoldTransformNode extends DataFlowNode { | |||
| }, {}); | |||
| } | |||
|
|
|||
| private getNames(): [string, string] | undefined { | |||
TODO: