ARROW-208: Add checkstyle policy to java project

[oza]

No description provided.

[oza]

Adding a google-style checkstyle

[julienledem]

LGTM.
@StevenMPhillips: opinion?

[julienledem]

@oza: how come it passes without changing anything to the code?

[oza]

@julienledem thanks for your review and sorry for the delay. The result of mvn checkstyle:checkstyle dumps over 1400 warnings. Can we fix it on another jira?

[wesm]

Should probably use this JIRA to fix the styles

[oza]

Sure, I will send a PR to fix the styles here.

[julienledem]

@oza depending on what the issues are you can also add some excludes.

[oza]

@julienledem fixing coding style which we can fix with IDE straightforwardly. Could you check it?

About vector directory, we should also check LOC and import ordering, but it cannot do with java configuration.

[oza]

I sent PR with wrong pom.xml configuration by my mistake. Resent PR.

[julienledem]

+1 LGTM
Sorry, this needs to be rebased.

[julienledem]

to ease the transition, I'd increase the column limit a bit.

[wesm]

Can this be rebased?

[wesm]

@oza can you rebase? Thanks

[wesm]

@oza rebase?

[wesm]

Happy new year. Can you rebase?

[julienledem]

@oza if you rebase we'll merge. Also to ease the transition, I'd increase the column limit a bit.

[wesm]

@julienledem @jacques-n it looks like the contributor isn't coming back. Can someone from the Java side pick this up and make a new patch? I'd like to be able to write patches for the Java codebase with confidence that I'm not messing up the code style, but I'm not very savvy when it comes to editor configuration.

[kiril-me]

I have started to look at checkstyles. Google check style is strict. Should we change it:

• increase line size to 120
• change method parameter name and local variable name regex to ^a-z?$ (we have nSize, nLogged variables)
• Allow newline in Javadoc
• Javadoc optional or required?
• Method name can start with _

[wesm]

@kiril-me seems OK to me. 120 characters seems quite wide, what is the norm in other ASF Java projects (100 seems like a reasonable max -- in C++ we are limiting to 90)?

@julienledem we should prioritize this patch because other patches (e.g. #334) are having style issues

[oza]

@julienledem sorry for the delay. I would like to rebase the patch with 100 LOC based on @wesm's comment.

[oza]

• Rethinking of enabling failOnViolation and failsOnError, because there are thousands of code style violations and errors. Instead of doing it here, I just added option, checkstyle.failOnViolation. We can enable it by using -D option like this: mvn clean install -DskipTests -Dcheckstyle.failOnViolation.
• Rethinking of fixing checkstyle warnings here. Hence, I just uploaded a change against root pom.xml. Let's do this on separate issue. If it's time to fix, I would like to work on that with other contributors.

What do you think?

[wesm]

We'd probably want to wait until #334 is done before doing the style changes.

[oza]

Sure. Please ping me if I miss #334 is merged.

[wesm]

hi @oza now that #334 has been merged, would you mind completing this patch? Thank you!!

[oza]

The test failure is caused by being unable to download guava. It looks unrelated to me. Let me try to resubmitting tests again(by doing git push).

[ERROR]     Unresolveable build extension: Plugin kr.motd.maven:os-maven-plugin:1.5.0.Final or one of its dependencies could not be resolved: Could not transfer artifact com.google.guava:guava:jar:10.0.1 from/to central (http://repo.maven.apache.org/maven2): Connection to http://repo.maven.apache.org refused: Connection timed out -> [Help 2]

[wesm]

@oza can you make the style fixes in this patch?

[oza]

@wesm Okay, I formatted 80% of code in 809e729 with IDE. There are still javadoc and format warnings, which cannot be reformatted with IDE, but it's much get better formatted. I think it would be better to complete to fix in another jira. About vector code, they seem to be generated with template. This issue, IMHO, should be solved in another jira, too.

[wesm]

Thanks @oza -- @julienledem could you take a look?

Given we don't have many outstanding Java patches, it might be worth going ahead with the 120 -> 100 line length change right now, but we should try not to let this linger because there is other Java work pending.

The line wrapping in code comments seems a bit odd to me, this might merit some manual fixing at some point.

[julienledem]

does this mean that we don't run it at all?
can we just exclude the remaining errors?
we can exclude the generated code for now for example.

[julienledem]

@oza Fixing remaining issues in another JIRA sounds good. I think you can add exclusions for the generated code for now.
@wesm I think we can improve the comments formatting as we go. This PR is big enough as it is and a pain to maintain.
The travis failure seems unrelated.