removing schema references from vis types #20489
Conversation
ppisljar
added
WIP
💔 Build Failed |
ppisljar
force-pushed
the
fix/removeSchemaFromAggTypes
branch
from
d5b245f to
dc5df37
Compare
💔 Build Failed |
ppisljar
force-pushed
the
fix/removeSchemaFromAggTypes
branch
from
dc5df37 to
954b9c2
Compare
💔 Build Failed |
ppisljar
force-pushed
the
fix/removeSchemaFromAggTypes
branch
from
954b9c2 to
1de5a16
Compare
💔 Build Failed |
|
x-pack failiure jenkins, test this |
💚 Build Succeeded |
| @@ -105,7 +105,7 @@ export class MetricVisComponent extends Component { | |||
| table.columns.forEach((column, columnIndex) => { | |||
| const aggConfig = column.aggConfig; | |||
|
|
|||
| if (aggConfig && aggConfig.schema.group === 'buckets') { | |||
| if (aggConfig && aggConfig.type.type === 'buckets') { | |||
There was a problem hiding this comment.
😞type.type I'm not a fan of repetitions. I've just had a conversation with @timroes about headaches created by the namings of AggConfig, AggConfigs, AggParam, AggParams, aggs, agg, AggType etc.
I'm writing aloud here:
so the reason about having buckets and metrics is clear: metrics agg types are used as dependant variable (the usual y value of an expression) and the buckets are use as independent variable (usually the x variable in the expression).
We have many possible aggtypes for each dependant and independant variable. So the initial way of call it group could be a better or at least more readable name for this , and you will call it like: aggConfig.type.group. Can me also some more specific like groupId since buckets and metrics are identifiers of the group and not general variables.
What do you think?
There was a problem hiding this comment.
i agree, however i would not put that in this PR
There was a problem hiding this comment.
So the initial way of call it group could be a better or at least more readable name
i agree, however i would not put that in this PR
I am not sure I follow why changing type.type to type.group would not be part of this PR yet this is the PR that is changing aggConfig.schema.group to aggConfig.type.type.
There was a problem hiding this comment.
as the changes from aggConfig.schema.group to aggConfig.type.type do not include all the references to type.type it would increase the size of already big pr ...
i will open one to do this rename as soon as i get back from vacation i promise :)
There was a problem hiding this comment.
Thanks for clarifying. Makes sense
| @@ -1,132 +0,0 @@ | |||
| /* | |||
There was a problem hiding this comment.
Are these obsolete tests? Do we need to substitute them with new tests?
There was a problem hiding this comment.
these are tests for the agg_param_writer class which has not been in use for a while.
i removed it and also its tests.
There was a problem hiding this comment.
I am confused, I think that was actually just a helper class for testing if parameter writing works properly? At least those tests imho still had a use-case. I actually added a couple of new tests in #20519 which seemed to work fine to test, if the parameter write functions do what they should do.
If we remove that version of those test, I think we should build some replacement, for checking if parameter writing will actually return in the correct results.
ppisljar
force-pushed
the
fix/removeSchemaFromAggTypes
branch
from
1de5a16 to
a3145fc
Compare
💚 Build Succeeded |
💚 Build Succeeded |
| @@ -69,7 +69,7 @@ export class VisualizeDataLoader { | |||
| return this._visData; | |||
| } | |||
| catch (e) { | |||
| this.props.searchSource.cancelQueued(); | |||
There was a problem hiding this comment.
props actually has a searchSource reference. Shouldn't we rather use that one, to not make more references to Vis?
💚 Build Succeeded |
💔 Build Failed |
Part of #19813 effort
removes agg.schema references from agg_types (and some other places)
note:
aggConfig.schema.group === aggConfig.type.type