[BEAM-4303] Enforce ErrorProne analysis in examples project

[cademarkegard]

• Enforces the ErrorProne analysis on examples project
• Fixes the issues in order to enable failOnWarning

[iemejia]

R: @swegner @iemejia

[iemejia]

@swegner is there a way to mark ignores at the module level ?
I think we should use that only in this case StringSplitter

We should not expose guava's Splitter in the most basic examples, e.g. Wordcount/WindowedWordcount (because those are commonly copy pasted) and using the @SuppressWarnings("StringSplitter") also does not seem natural for a baginner example too so better do it at the module level.

[cademarkegard]

Great point. I can update the string splits to have a limit of -1 instead and that will pass ErrorProne then.

[swegner]

@iemejia currently we have a global set of excludes in the build_rules.gradle applyJavaNature() block. We could add a parameter to the JavaNatureConfiguration such that modules can add their own, but my preference is to avoid that until we really need it.

I agree with making examples simple and avoiding a Guava dependency, but also note that examples tend to get copy-pasted many times, so we should try to avoid known anti-patterns if possible. As @cademarkegard points out, the StringSplitter ErrorProne page also suggests using String.split(String, int) with a limit of -1.

[swegner]

Can you elaborate on what this change is for? I'm not sure what this magic number represents. If you are familiar, perhaps extract it out as a named constant that provides some context. i.e. const int NUM_FIELDS = 50;

[cademarkegard]

This value change is necessary after changing the c.element().split() to have limit of -1. In the second input data, when we didn't have the limit set, split() would actually truncate the fields following after the last 0 on line 61 resulting in length of 45. After setting the limit, it increased the number of fields to 49 for that input. We still maintained 50 fields for the input that starts on line 59, which textExtractTotalFlow() seems to care about.

Great suggestion on the constant, I'll push up the change with that now.

[swegner]

Got it, thanks for the explanation!

[swegner]

Just one comment above, then everything else LGTM (looks good to me).

[cademarkegard]

@iemejia @swegner I've addressed the feedback and the tests pass. Please take a look when you have a chance.

[swegner]

LGTM.

I'll leave it to @iemejia to merge in case he has any other feedback.

[iemejia]

LGTM. Thanks @cademarkegard !