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.

Sign up for GitHub

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

Jump to bottom

remove vis.aggs references from aggTypes #20508

Merged
merged 4 commits into from
Jul 11, 2018
Merged

remove vis.aggs references from aggTypes #20508

merged 4 commits into from
Jul 11, 2018

Conversation

ppisljar
Copy link
Member

@ppisljar ppisljar commented Jul 6, 2018 •
edited
Loading

remove vis.aggs references from aggTypes

  • AggConfigs is passed to AggConfig.toDsl which passes it to AggParams.write which passes it to AggParam.write
  • aggConfig stores reference to AggConfigs in _aggs property for a few usecases where it needs to access it out of param.write function, so it no longer needs to be accessed from vis object.

in UI (controllers, directives) we still have reference to vis.params, which is not problematic as vis is available on the scope there.

preparation for #19813

@ppisljar ppisljar added WIP Work in progress Feature:Visualizations Generic visualization features (in case no more specific feature label is available) labels Jul 6, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

@ppisljar ppisljar force-pushed the fix/aggTypesRemoveVisAggs branch 2 times, most recently from da627a8 to 2a94cb4 Compare July 9, 2018 06:54
@elasticmachine
Copy link
Contributor

💔 Build Failed

@ppisljar ppisljar force-pushed the fix/aggTypesRemoveVisAggs branch 2 times, most recently from 3610328 to aba2bff Compare July 9, 2018 12:03
@elasticmachine
Copy link
Contributor

💔 Build Failed

@ppisljar ppisljar force-pushed the fix/aggTypesRemoveVisAggs branch from aba2bff to 711be6c Compare July 9, 2018 13:40
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@ppisljar ppisljar force-pushed the fix/aggTypesRemoveVisAggs branch from 711be6c to e90c562 Compare July 9, 2018 17:43
@ppisljar ppisljar added review v6.4.0 chore and removed WIP Work in progress labels Jul 9, 2018
@ppisljar ppisljar requested review from timroes, markov00 and nreese July 9, 2018 17:43
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

markov00
markov00 requested changes Jul 10, 2018
View reviewed changes
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

Tested the code on Chrome/OSX. Everything works as expected.
I've left some comment to address if possibly before merge

src/ui/public/agg_types/metrics/lib/parent_pipeline_agg_writter.js Outdated
const parentPipelineAggWritter = function (agg, output) {
const vis = agg.vis;
const selectedMetric = agg.params.customMetric || vis.aggs.getResponseAggById(agg.params.metricAgg);
const parentPipelineAggWritter = function (agg, output, aggs) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this a typo writter?

Copy link
Member

Choose a reason for hiding this comment

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

Also the file name is writter, sould be writer

src/ui/public/vis/agg_configs.js Outdated
});

this.push.apply(this, configStates.map(aggConfigState => {
if (aggConfigState instanceof AggConfig) return aggConfigState;
Copy link
Member

Choose a reason for hiding this comment

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

could you please add braces here?

src/ui/public/vis/agg_config.js
this.id = String(opts.id || AggConfig.nextId(vis.aggs));
this.vis = vis;
this._indexPattern = vis.indexPattern;
this._aggs = aggs || vis.aggs;
Copy link
Member

Choose a reason for hiding this comment

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

this._aggs is not used inside the AggConfig class, we can remove it so we can get rid of having stored references of it's AggConfigs array inside each object.

Copy link
Member Author

Choose a reason for hiding this comment

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

it is, there are some references to aggConfig (or config)._aggs we can't remove just yet

Copy link
Contributor

Choose a reason for hiding this comment

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

Will we work towards removing it completely? Because if we should access it from the outside, we should rather not name this like a private variable?

src/ui/public/vis/agg_config.js Outdated
@@ -176,9 +177,9 @@ class AggConfig {
* @return {void|Object} - if the config has a dsl representation, it is
* returned, else undefined is returned
*/
toDsl() {
toDsl(aggs) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you please also update the doc block here? on the doc the param is called @param {AggConfig} aggConfig . Or maybe change the aggs param to aggConfig ti keep it a bit more readable

@ppisljar ppisljar force-pushed the fix/aggTypesRemoveVisAggs branch from 25cd0b0 to b5d9100 Compare July 10, 2018 12:38
markov00
markov00 reviewed Jul 10, 2018
View reviewed changes
src/ui/public/vis/agg_config.js Outdated
@@ -172,13 +173,13 @@ class AggConfig {
*
* Adds params and adhoc subaggs to a pojo, then returns it
*
* @param {AggConfig} aggConfig - the config object to convert
* @param {aggConfigs} aggConfigs - the config object to convert
Copy link
Member

Choose a reason for hiding this comment

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

Sould be AggConfigsas param type

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

timroes
timroes reviewed Jul 10, 2018
View reviewed changes
src/ui/public/vis/agg_configs.js Outdated
});

this.push.apply(this, configStates.map(aggConfigState => {
Copy link
Contributor

Choose a reason for hiding this comment

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

this.push(...configStates.map(...))

timroes
timroes approved these changes Jul 10, 2018
View reviewed changes
Copy link
Contributor

@timroes timroes left a comment

Choose a reason for hiding this comment

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

Just some minor nitpicks, LGTM

@ppisljar ppisljar merged commit 5042c38 into elastic:master Jul 11, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

ppisljar added a commit to ppisljar/kibana that referenced this pull request Jul 11, 2018
@ppisljar
remove vis.aggs references from aggTypes (elastic#20508)
4159413 
# Conflicts:
#	src/ui/public/agg_types/buckets/date_histogram.js
@ppisljar ppisljar mentioned this pull request Jul 11, 2018
Merged
ppisljar added a commit that referenced this pull request Jul 11, 2018
@ppisljar
remove vis.aggs references from aggTypes (#20508) (#20667)
79a8e47
@elasticmachine
Copy link
Contributor

💔 Build Failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markov00 markov00 markov00 approved these changes

@timroes timroes timroes approved these changes

@nreese nreese Awaiting requested review from nreese

Assignees
No one assigned
Labels
chore Feature:Visualizations Generic visualization features (in case no more specific feature label is available) review v6.4.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ppisljar @elasticmachine @timroes @markov00