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.

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

Remove Settings.settingsBuilder. #17619

Merged
merged 1 commit into from Apr 8, 2016

Conversation

jpountz
Copy link
Contributor

@jpountz jpountz commented Apr 8, 2016

We have both Settings.settingsBuilder and Settings.builder that do exactly
the same thing, so we should keep only one. I kept Settings.builder since it
has my preference but also it is the one that we use in examples of the Java API.

@nik9000
Copy link
Member

nik9000 commented Apr 8, 2016

Settings.builder

@nik9000 nik9000 self-assigned this Apr 8, 2016
@jpountz
Copy link
Contributor Author

jpountz commented Apr 8, 2016

Note to reviewers: the change is purely mechanical.

@@ -86,7 +86,7 @@
import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_NUMBER_OF_REPLICAS;
import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_NUMBER_OF_SHARDS;
import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_VERSION_CREATED;
import static org.elasticsearch.common.settings.Settings.settingsBuilder;
import static org.elasticsearch.common.settings.Settings.builder;
Copy link
Member

Choose a reason for hiding this comment

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

We probably should never import static Strings.builder because Settings.builder() is much more readable than builder(). It'd be fairly simple to add a check for that to the regexes we run during checkstyle. I'd probably do that - either temporarily to find and remove them all or permanently with a helpful message telling people to use Settings.builder.

BTW, I love static imports when the static methods are named appropriately, but I also love when static methods can be named in such a way that it is nicer not to statically import them. So, personally, I like that we don't have a hard and fast rule against static imports. The SETTINGS above this are a super great use of static imports.

@nik9000
Copy link
Member

nik9000 commented Apr 8, 2016

In the name of moving forward, LGTM. but we really really shouldn't be statically importing Settings.builder because it makes the call sites confusing. I'm fine with merging this as is and creating another PR myself to remove those imports if you want.

@dadoonet
Copy link
Member

dadoonet commented Apr 8, 2016

++ Thanks for doing this Adrien! This always confuses me when my IDE propose the 2 methods... :)

@jpountz
Copy link
Contributor Author

jpountz commented Apr 8, 2016

@nik9000 I agree these static imports are not nice. I pushed a new commit.

@nik9000
Copy link
Member

nik9000 commented Apr 8, 2016

LGTM

We have both `Settings.settingsBuilder` and `Settings.builder` that do exactly
the same thing, so we should keep only one. I kept `Settings.builder` since it
has my preference but also it is the one that we use in examples of the Java API.
@jpountz jpountz merged commit 42526ac into elastic:master Apr 8, 2016
@jpountz jpountz deleted the remove/settingsBuilder branch April 8, 2016 16:13
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.

None yet

4 participants