Core: Drop nodeName from AbstractComponent#34487
Merged
nik9000 merged 3 commits intoelastic:masterfrom Oct 26, 2018
Merged
Conversation
`AbstractComponent` is trouble because its name implies that *everything* should extend from it. It *is* useful, but maybe too broadly useful. The things it offers access too, the `Settings` instance for the entire server and a logger are nice to have around, but not really needed *everywhere*. The `Settings` instance especially adds a fair bit of ceremony to testing without any value. This removes the `nodeName` method from `AbstractComponent` so it is more clear where we actually need the node name.
Collaborator
|
Pinging @elastic/es-core-infra |
nik9000
added a commit
that referenced
this pull request
Oct 27, 2018
`AbstractComponent` is trouble because its name implies that *everything* should extend from it. It *is* useful, but maybe too broadly useful. The things it offers access too, the `Settings` instance for the entire server and a logger are nice to have around, but not really needed *everywhere*. The `Settings` instance especially adds a fair bit of ceremony to testing without any value. This removes the `nodeName` method from `AbstractComponent` so it is more clear where we actually need the node name.
Member
Author
|
Merged and backported! Thanks @rjernst! |
kcm
pushed a commit
that referenced
this pull request
Oct 30, 2018
`AbstractComponent` is trouble because its name implies that *everything* should extend from it. It *is* useful, but maybe too broadly useful. The things it offers access too, the `Settings` instance for the entire server and a logger are nice to have around, but not really needed *everywhere*. The `Settings` instance especially adds a fair bit of ceremony to testing without any value. This removes the `nodeName` method from `AbstractComponent` so it is more clear where we actually need the node name.
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
4 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.
AbstractComponentis trouble because its name implies thateverything should extend from it. It is useful, but maybe too
broadly useful. The things it offers access too, the
Settingsinstancefor the entire server and a logger are nice to have around, but not
really needed everywhere. The
Settingsinstance especially adds afair bit of ceremony to testing that is usually confusing to read.
This removes the
nodeNamemethod fromAbstractComponentso it ismore clear where we actually need the node name.
Relates to #34488.