Cleanup suppressed overlength line for action.support package by not-napoleon · Pull Request #34889 · elastic/elasticsearch

not-napoleon · 2018-10-25T21:33:19Z

Clean up lines over 140 characters in the org.elasticsearch.action.support.* packages (master branch version)

Relates to #34884

dakrone

LGTM assuming CI is happy :)

AthenaEryma

Left a few nits that you can either take or not as you like, overall LGTM.

AthenaEryma · 2018-10-25T21:52:39Z

...n/java/org/elasticsearch/action/support/replication/TransportBroadcastReplicationAction.java

            if (indexMetaData != null) {
-                for (IntObjectCursor<IndexShardRoutingTable> shardRouting : clusterState.getRoutingTable().indicesRouting().get(index).getShards()) {
+                for (IntObjectCursor<IndexShardRoutingTable> shardRouting :
+                    clusterState.getRoutingTable().indicesRouting().get(index).getShards()) {


I did this kind of thing as

for (IntObjectCursor<IndexShardRoutingTable> shardRouting : clusterState.getRoutingTable().indicesRouting().get(index).getShards()) {

Since I thought it made it more visually obvious that the second line was part of the for and not the code block below, but I could see either way.

+1. I really like indenting an extra level for things like this so the for loop's "for" parts don't line up with the block's body.

AthenaEryma · 2018-10-25T21:54:05Z

...org/elasticsearch/action/support/single/instance/TransportInstanceSingleOperationAction.java

-public abstract class TransportInstanceSingleOperationAction<Request extends InstanceShardOperationRequest<Request>, Response extends ActionResponse>
-        extends HandledTransportAction<Request, Response> {
+public abstract class TransportInstanceSingleOperationAction<Request extends InstanceShardOperationRequest<Request>,
+        Response extends ActionResponse> extends HandledTransportAction<Request, Response> {


I'd prefer:

Suggested change

Response extends ActionResponse> extends HandledTransportAction<Request, Response> {

Response extends ActionResponse>

extends HandledTransportAction<Request, Response> {

But again, either way really.

public abstract class TransportInstanceSingleOperationAction< Request extends InstanceShardOperationRequest<Request>, Response extends ActionResponse > extends HandledTransportAction<Request, Response> {

Is my preference. Stately.

AthenaEryma · 2018-10-25T21:55:38Z

.../src/main/java/org/elasticsearch/action/support/single/shard/TransportSingleShardAction.java

                }
-                transportService.sendRequest(node, transportShardAction, internalRequest.request(), new TransportResponseHandler<Response>() {
+                transportService.sendRequest(node, transportShardAction, internalRequest.request(), new TransportResponseHandler<Response>()
+                {


I think moving the whole new new TransportResponseHandler<Response>() { onto the next line is a little clearer.

nik9000

I've left a few minor things but also LGTM.

nik9000 · 2018-10-25T22:06:57Z

server/src/main/java/org/elasticsearch/action/support/DelegatingActionListener.java


-public abstract class DelegatingActionListener<Instigator extends ActionResponse, Delegated extends  ActionResponse> implements ActionListener<Instigator> {
+public abstract class DelegatingActionListener<Instigator extends ActionResponse, Delegated extends  ActionResponse>
+        implements ActionListener<Instigator> {


nik9000 · 2018-10-25T22:07:37Z

...c/main/java/org/elasticsearch/action/support/broadcast/BroadcastOperationRequestBuilder.java

 import org.elasticsearch.client.ElasticsearchClient;

-public abstract class BroadcastOperationRequestBuilder<Request extends BroadcastRequest<Request>, Response extends BroadcastResponse, RequestBuilder extends BroadcastOperationRequestBuilder<Request, Response, RequestBuilder>>
+public abstract class BroadcastOperationRequestBuilder<Request extends BroadcastRequest<Request>, Response extends BroadcastResponse,


I'd probably do these one-per-line at this point.

nik9000 · 2018-10-25T22:07:43Z

server/src/main/java/org/elasticsearch/action/support/broadcast/TransportBroadcastAction.java


-public abstract class TransportBroadcastAction<Request extends BroadcastRequest<Request>, Response extends BroadcastResponse, ShardRequest extends BroadcastShardRequest, ShardResponse extends BroadcastShardResponse>
+public abstract class TransportBroadcastAction<Request extends BroadcastRequest<Request>, Response extends BroadcastResponse,
+        ShardRequest extends BroadcastShardRequest, ShardResponse extends BroadcastShardResponse>


nik9000 · 2018-10-25T22:08:37Z

server/src/main/java/org/elasticsearch/action/support/broadcast/TransportBroadcastAction.java

                        onOperation(shard, shardIt, shardIndex, new NoShardAvailableActionException(shardIt.shardId()));
                    } else {
-                        transportService.sendRequest(node, transportShardAction, shardRequest, new TransportResponseHandler<ShardResponse>() {
+                        transportService.sendRequest(node, transportShardAction, shardRequest, new TransportResponseHandler<ShardResponse>()


Maybe put the class on a new line rather than move the body opening like this.

nik9000 · 2018-10-25T22:10:00Z

...org/elasticsearch/action/support/single/instance/TransportInstanceSingleOperationAction.java

-public abstract class TransportInstanceSingleOperationAction<Request extends InstanceShardOperationRequest<Request>, Response extends ActionResponse>
-        extends HandledTransportAction<Request, Response> {
+public abstract class TransportInstanceSingleOperationAction<Request extends InstanceShardOperationRequest<Request>,
+        Response extends ActionResponse> extends HandledTransportAction<Request, Response> {


public abstract class TransportInstanceSingleOperationAction< Request extends InstanceShardOperationRequest<Request>, Response extends ActionResponse > extends HandledTransportAction<Request, Response> {

Is my preference. Stately.

nik9000 · 2018-10-25T22:10:19Z

.../src/main/java/org/elasticsearch/action/support/single/shard/TransportSingleShardAction.java

                }
-                transportService.sendRequest(node, transportShardAction, internalRequest.request(), new TransportResponseHandler<Response>() {
+                transportService.sendRequest(node, transportShardAction, internalRequest.request(), new TransportResponseHandler<Response>()
+                {


nik9000 · 2018-10-25T22:11:02Z

...er/src/test/java/org/elasticsearch/action/support/replication/BroadcastReplicationTests.java

        TestBroadcastReplicationAction(Settings settings, ClusterService clusterService, TransportService transportService,
                ActionFilters actionFilters, IndexNameExpressionResolver indexNameExpressionResolver,
-                TransportReplicationAction<BasicReplicationRequest, BasicReplicationRequest, ReplicationResponse> replicatedBroadcastShardAction) {
+                TransportReplicationAction<BasicReplicationRequest, BasicReplicationRequest, ReplicationResponse>


Wow! I've never thought of breaking the type from the name before but it looks like we kind of have to to fit it in.

yeah, I'm really not happy with doing it this way, but the only alternative I saw was to split the type parameters out to separate lines, which looked worse to me. I'm open to suggestions.

colings86 · 2018-10-26T08:55:06Z

Please remember to add all relevant labels (area label, version label(s) and change type label) on all PRs and please look for this as part of reviews. The release note generation process is made much harder when PRs are not labelled correctly.

elasticmachine · 2018-10-26T08:55:58Z

Pinging @elastic/es-core-infra

Conflicts: server/src/main/java/org/elasticsearch/action/support/broadcast/TransportBroadcastAction.java server/src/main/java/org/elasticsearch/action/support/single/shard/TransportSingleShardAction.java

Clean up lines over 140 characters in the `org.elasticsearch.action.support.*` packages Relates to #34884

Cleanup suppressed overlength line for action.support package

e16715e

not-napoleon added >non-issue >refactoring v7.0.0 labels Oct 25, 2018

dakrone mentioned this pull request Oct 25, 2018

Remove checkstyle line length suppressions #34884

Closed

33 tasks

dakrone approved these changes Oct 25, 2018

View reviewed changes

AthenaEryma approved these changes Oct 25, 2018

View reviewed changes

nik9000 approved these changes Oct 25, 2018

View reviewed changes

colings86 added the :Core/Infra/Core Core issues without another label label Oct 26, 2018

not-napoleon added 4 commits October 26, 2018 11:30

response to PR feedback

ab7bf48

fix some indentation that Intellij clobbered

4e3d727

rename long variable to avoid ugly line break

d46b95c

Merge branch 'master' into fix/line-length-cleanup-action-support

b77a4ce

Conflicts: server/src/main/java/org/elasticsearch/action/support/broadcast/TransportBroadcastAction.java server/src/main/java/org/elasticsearch/action/support/single/shard/TransportSingleShardAction.java

not-napoleon merged commit 329a94b into elastic:master Oct 29, 2018

not-napoleon deleted the fix/line-length-cleanup-action-support branch October 29, 2018 13:22

kcm pushed a commit that referenced this pull request Oct 30, 2018

Cleanup suppressed overlength line for action.support package (#34889)

676a452

Clean up lines over 140 characters in the `org.elasticsearch.action.support.*` packages Relates to #34884

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

	Response extends ActionResponse> extends HandledTransportAction<Request, Response> {
	Response extends ActionResponse>
	extends HandledTransportAction<Request, Response> {

Conversation

not-napoleon commented Oct 25, 2018

Uh oh!

dakrone left a comment

Choose a reason for hiding this comment

Uh oh!

AthenaEryma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colings86 commented Oct 26, 2018

Uh oh!

elasticmachine commented Oct 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants