Skip to content

Fix small mistake made in PR 6511#7034

Merged
ycombinator merged 3 commits into
elastic:masterfrom
ycombinator:gh-6511-fix
Apr 25, 2016
Merged

Fix small mistake made in PR 6511#7034
ycombinator merged 3 commits into
elastic:masterfrom
ycombinator:gh-6511-fix

Conversation

@ycombinator
Copy link
Copy Markdown
Contributor

In PR #6511, I attempted to fix the issue where custom labels were not being honored by the standard deviation metric aggregation. However, I did not implement the fix exactly as specified in bullet point 1 in #6511 (comment). This PR fixes the implementation.

@ycombinator
Copy link
Copy Markdown
Contributor Author

@spalger I'm assigning this review to you because you did the review for the original PR #6511.

makeLabel: function () {
let details = this.keyedDetails(this.params.customLabel)[this.key];
return details.title + ' of ' + this.fieldDisplayName();
return this.keyedDetails(this.params.customLabel, this.fieldDisplayName())[this.key].title;
Copy link
Copy Markdown
Contributor

@spalger spalger Apr 24, 2016

Choose a reason for hiding this comment

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

I think this should be broken into a few lines and use lodash's get function like:

const fieldDisplay = this.fieldDisplayName();
const details = this.keyedDetails(this.param.customLabel, fieldDisplay);
return get(details, [this.key, 'title']);

Since the keyedDetails() function is defined so close by it probably won't matter, but it would make me feel better.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I thought about breaking it up too, for better readability. Thanks for nudging me over the fence :)

I'll use lodash's get function but could you educate me as to why that's preferable?

@spalger
Copy link
Copy Markdown
Contributor

spalger commented Apr 24, 2016

LGTM

@spalger spalger assigned ycombinator and unassigned spalger Apr 24, 2016
@ycombinator ycombinator merged commit a37bab0 into elastic:master Apr 25, 2016
@ycombinator ycombinator deleted the gh-6511-fix branch April 25, 2016 01:04
patrykkopycinski pushed a commit to patrykkopycinski/kibana that referenced this pull request May 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants