Skip to content

Conversation

@matriv
Copy link
Contributor

@matriv matriv commented Jan 8, 2019

Fixes: #36532

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

@matriv
Copy link
Contributor Author

matriv commented Jan 8, 2019

There are still a few occurrences where the String.format() is required like: https://github.com/elastic/elasticsearch/blob/master/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java#L559

And also here: https://github.com/elastic/elasticsearch/blob/master/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/ProtoUtils.java#L44 for which we would need to add dependency to server-main, which I guess we shouldn't do as proto is a dependency of jdbc and we don't want to blow it up by adding the server-main to it.

@matriv
Copy link
Contributor Author

matriv commented Jan 8, 2019

run default distro tests

@matriv
Copy link
Contributor Author

matriv commented Jan 8, 2019

run gradle build tests 1

@Override
public String toString() {
return String.format(Locale.ROOT, "{%s=%s}", prefix(), value);
return LoggerMessageFormat.format(null, "{{}={}}", prefix(), value);
Copy link
Member

@costin costin Jan 8, 2019

Choose a reason for hiding this comment

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

Why use the null prefix -and not use alternative method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@costin I use the one with the prefix and using null because of signature mixup:
String, String calls the one with the prefix and not the String, Object... one...

Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

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

LGTM - I assumed the format appears in more places.
No need to add imports into the other projects.

P.S. format is a great candidate for static importing.

Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

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

LGTM

@matriv
Copy link
Contributor Author

matriv commented Jan 8, 2019

run gradle build tests 1

@matriv
Copy link
Contributor Author

matriv commented Jan 8, 2019

run default distro tests

@matriv
Copy link
Contributor Author

matriv commented Jan 8, 2019

run gradle build tests 1

@matriv matriv merged commit 5f2fbed into elastic:master Jan 8, 2019
@matriv matriv deleted the mt/fix-36532 branch January 8, 2019 21:56
@matriv
Copy link
Contributor Author

matriv commented Jan 8, 2019

Backported to 6.x with 7b8b405

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.

5 participants