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

Fix compiler warnings in :server - part 3 #76024

Merged

Conversation

pugnascotia
Copy link
Contributor

Part of #40366. Fix a number of javac issues when linting is enforced in server/. Due to the number of issues that this surfaces, the fixes will be spread of multiple PRs.

@pugnascotia pugnascotia requested a review from mark-vieira August 3, 2021 13:25
@elasticmachine elasticmachine added the Team:Delivery Meta label for Delivery team label Aug 3, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-delivery (Team:Delivery)

@pugnascotia
Copy link
Contributor Author

Poke @mark-vieira

Copy link
Contributor

@mark-vieira mark-vieira left a comment

Choose a reason for hiding this comment

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

Sorry for the slow review. LGTM.

Are we there yet? 😉

@@ -126,6 +126,7 @@ private Response newResponse(
exceptions.add(new DefaultShardOperationFailedException(shard.getIndexName(), shard.getId(), exception));
}
} else {
@SuppressWarnings("unchecked")
NodeResponse response = (NodeResponse) responses.get(i);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does forciblyCast not work here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well it would, but I don't really like using it - I feel like the annotation draws attention more than a function call, even with its name, and I'd prefer the engineers to want to make such cast unnecessary wherever possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should have some clear guidance here then on when one is preferable over the other. As it is, it doesn't seem obvious to me. My intention behind adding that method was to eliminate the need for this suppressions all over the case when there's no alternative to casting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added something to CONTRIBUTING.md, see what you think.

@pugnascotia
Copy link
Contributor Author

@elasticmachine update branch

@pugnascotia pugnascotia merged commit 128a7e7 into elastic:master Aug 10, 2021
@pugnascotia pugnascotia deleted the 40366-lint-warnings-server-pt3 branch August 10, 2021 14:05
pugnascotia added a commit that referenced this pull request Aug 10, 2021
Part of #40366. Fix a number of javac issues when linting is enforced in `server/`.
@pugnascotia
Copy link
Contributor Author

Backported to 7.x in eb2f4c8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Build Build or test infrastructure >refactoring Team:Delivery Meta label for Delivery team v7.15.0 v8.0.0-alpha2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants