Skip to content

[BEAM-4326] Enforce ErrorProne analysis in the fn-execution project#5542

Merged
iemejia merged 1 commit intoapache:masterfrom
cademarkegard:beam4326/errorprone-fn-execution
Jun 7, 2018
Merged

[BEAM-4326] Enforce ErrorProne analysis in the fn-execution project#5542
iemejia merged 1 commit intoapache:masterfrom
cademarkegard:beam4326/errorprone-fn-execution

Conversation

@cademarkegard
Copy link
Contributor

  1. Enables failOnWarning for fn-execution project
  2. Cleans up related code to get error prone to pass

@cademarkegard cademarkegard force-pushed the beam4326/errorprone-fn-execution branch from c05076e to a6f26ed Compare June 2, 2018 05:27
@cademarkegard
Copy link
Contributor Author

R: @swegner @iemejia

Copy link
Contributor

@swegner swegner left a comment

Choose a reason for hiding this comment

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

Looks pretty good; as a general comment, it's valuable to have a comment explaining analysis suppressions. That let's future readers understand why the suppression is safe, and to re-evaluate whether it still applies as the code gets maintained.

}

@Test
@SuppressWarnings("FutureReturnValueIgnored")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a brief comment describing why this suppression is safe?

}

@Test
@SuppressWarnings("FutureReturnValueIgnored")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a brief comment describing why this suppression is safe?

@cademarkegard cademarkegard force-pushed the beam4326/errorprone-fn-execution branch from a6f26ed to 107287e Compare June 6, 2018 00:08
@cademarkegard
Copy link
Contributor Author

run java precommit

@cademarkegard
Copy link
Contributor Author

run python precommit

@cademarkegard
Copy link
Contributor Author

@swegner thanks for the review! I addressed the pending items and got the tests to pass.

@swegner
Copy link
Contributor

swegner commented Jun 6, 2018

Thanks, this looks good to me!

Passing over to a committer to merge.. @lukecwik can you help?

@cademarkegard
Copy link
Contributor Author

thanks @swegner !

Copy link
Member

@iemejia iemejia left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@iemejia iemejia merged commit b118bfb into apache:master Jun 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants