[aggs] use getFieldDisplayName and getField helpers#8717
Merged
spalger merged 4 commits intoelastic:masterfrom Oct 18, 2016
Merged
[aggs] use getFieldDisplayName and getField helpers#8717spalger merged 4 commits intoelastic:masterfrom
spalger merged 4 commits intoelastic:masterfrom
Conversation
spalger
changed the title
spalger
force-pushed
the
cleanup/use-field-displayname-getter
branch
from
688c02b to
51cc35b
Compare
tylersmalley
requested changes
Oct 17, 2016
Member
tylersmalley
left a comment
tylersmalley
left a comment
There was a problem hiding this comment.
makeLabel in https://github.com/elastic/kibana/blob/master/src/ui/public/agg_types/metrics/percentile_ranks.js#L30 is referencing the now obsolete fieldDisplayName()
tylersmalley
approved these changes
Oct 18, 2016
Member
tylersmalley
left a comment
tylersmalley
left a comment
There was a problem hiding this comment.
LGTM. There are some opportunities for cleanup but are outside of the scope of these changes.
- makeLabel could have consistent naming for the argument passed (aggConfig vs agg).
- consistent naming for variable assignment from display names. For example: https://github.com/elastic/kibana/pull/8717/files#diff-2cd930ccd69e832107c9578ef648e5b7R44 could be fieldDisplayName
Contributor
Author
|
👍 |
elastic-jasper
added a commit
that referenced
this pull request
Oct 18, 2016
--------- **Commit 1:** [aggConfig] use `getField`/`getFieldDisplayName` helpers * Original sha: 0190cda * Authored by spalger <email@spalger.com> on 2016-10-17T20:21:08Z **Commit 2:** [aggParams/field] throw better error when field missing * Original sha: 1ee969c * Authored by spalger <email@spalger.com> on 2016-10-17T20:33:26Z **Commit 3:** [tests] update tests that depended on `aggConfig#field()` * Original sha: 51cc35b * Authored by spalger <email@spalger.com> on 2016-10-17T21:26:31Z **Commit 4:** [aggTypes/percentile] fix reference to aggConfig.fieldDisplayName() * Original sha: ae32232 * Authored by spalger <email@spalger.com> on 2016-10-18T02:06:27Z
elastic-jasper
added a commit
that referenced
this pull request
Oct 18, 2016
--------- **Commit 1:** [aggConfig] use `getField`/`getFieldDisplayName` helpers * Original sha: 0190cda * Authored by spalger <email@spalger.com> on 2016-10-17T20:21:08Z **Commit 2:** [aggParams/field] throw better error when field missing * Original sha: 1ee969c * Authored by spalger <email@spalger.com> on 2016-10-17T20:33:26Z **Commit 3:** [tests] update tests that depended on `aggConfig#field()` * Original sha: 51cc35b * Authored by spalger <email@spalger.com> on 2016-10-17T21:26:31Z **Commit 4:** [aggTypes/percentile] fix reference to aggConfig.fieldDisplayName() * Original sha: ae32232 * Authored by spalger <email@spalger.com> on 2016-10-18T02:06:27Z
This was referenced Oct 18, 2016
airow
pushed a commit
to airow/kibana
that referenced
this pull request
Feb 16, 2017
--------- **Commit 1:** [aggConfig] use `getField`/`getFieldDisplayName` helpers * Original sha: a2d1986dc71d022084fded04f3a235333dce2258 [formerly 0190cda] * Authored by spalger <email@spalger.com> on 2016-10-17T20:21:08Z **Commit 2:** [aggParams/field] throw better error when field missing * Original sha: c6731d820815afb408ecd35ad9ace24a9b604b2a [formerly 1ee969c] * Authored by spalger <email@spalger.com> on 2016-10-17T20:33:26Z **Commit 3:** [tests] update tests that depended on `aggConfig#field()` * Original sha: 2a6b7f9dcca1db1b341a5273eb7381c0362fd92d [formerly 51cc35b] * Authored by spalger <email@spalger.com> on 2016-10-17T21:26:31Z **Commit 4:** [aggTypes/percentile] fix reference to aggConfig.fieldDisplayName() * Original sha: ab166f5c3c88bbe59fbb00487b15c526f3c39d86 [formerly ae32232] * Authored by spalger <email@spalger.com> on 2016-10-18T02:06:27Z Former-commit-id: 6c2a806
airow
pushed a commit
to airow/kibana
that referenced
this pull request
Feb 16, 2017
[backport] PR elastic#8717 to 5.x Former-commit-id: b4521f5
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While working to fix #8643 it became possible for a saved aggregation config to have an invalid field. Rather than put the invalid field back into the aggConfig, the field param is set to null and the form validation directs the user to specify a new field. This caused a slew of "Cannot read property XYZ of undefined" errors as there are many places directly accessing the field on the
aggConfig.params.This is why the
field()andfieldDisplayName()getters were defined, so I first updated those to use thegetXYZ()naming that we use everywhere else and they updated the components that were reaching in for these properties to use the getters.