Code formatting issues

[pkiraly]

I just found that Dataverse already have a checkstyle.xml. Most of the rules are commented out, but even the remaining produce 200+ reported violation of the ruleset.

The command to check errors:

mvn clean checkstyle:checkstyle

This generates the HTML page 'target/site/checkstyle.html'.

It is clear if you open any Java code, that the number of commiters working with different coding style, so the result is not uniformed. Clearer code does not improve the software, but helps lowering the burden of maintenance and making the code more readable. My past experiences with code clearing efforts suggest, that this is an ongoing process: you have to run checks again and again, and clearing code constantly.

Code cleaning on the other hand is an ideal task if you, as a novice in the community just would like to test the water and try to understand the structure of the code. It doesn't require of deep knowledge of the code, and still, you can contribute with some useful thing.

I see three subtasks here:

• run check, and submit pull requests for the clearer code
• extend the checks one by one, increasing the number of checkstyle ruleset
• improving the documentation about the coding style in Dataverse manual (http://guides.dataverse.org/en/4.9.4/developers/coding-style.html)

[pdurbin]

@pkiraly hi! Yes, checkstyle.xml was added quite recently in #5075. Probably we should have added when we first started the project but we didn't have @poikilotherm to provide the file for us.

In the "Coding Style" page you linked you can read my "Format Code You Changed with Netbeans" idea:

"It is much appreciated when you format your code (but only the code you touched!) using the out-of-the-box Netbeans configuration."

I realize that this process of only reformatting the code we touch means it will take a long time to get all the code to be consistent.

For more thoughts on formatting code, please see the "gofmt for Java" thread I started on the dataverse-dev list a while back: https://groups.google.com/d/msg/dataverse-dev/y2Jpk3szTf8/rckKmP6-BgAJ . It's unfortunate that Java doesn't have Go's culture of a consistent style that everyone using the language adheres to.

[pkiraly]

Meantime I've started working with the current checkstyle settings, and fixed all the problems. It covers about 200 Java source files.

[pdurbin]

@pkiraly um. I'm not sure I want all of that change at once. It's very disruptive to our git history. Can we please talk about this more?

[pdurbin]

@pkiraly oh. You already made pull request #5337. I'll drag it to code review at https://waffle.io/IQSS/dataverse and we'll talk about it at standup.

[pkiraly]

@pdurbin I am open to discuss it. If this approach doesn't fit for your workflow, we can do differently. The thing is that approx. 200 files have either tabs in it instead of spaces (most of them has sporadic tabs, and only a couple of files has only tabs) for indentation. The other type of issue the checkstyle check is the usage of new lines before right curly braces, such as

public String someMethod()
{
    ...
}

We could solve this problem package by package or in other smaller chunks.

[pdurbin]

@pkiraly thanks. Please be advised that standup was cancelled today due to light attendance and the next two days are a holiday (Thanksgiving). Thank you for helping us make the code more consistent. I'm curious about your thoughts on keeping it consistent over time.

[pkiraly]

For a long run checkstyle report could be part of continous integration. I don't know yet if there is a service for that like coverall for code coverage. Needs a bit investigation.

[pkiraly]

@pdurbin ... and happy holidays!

[poikilotherm]

As the one who opened #5075 in the first place, I can only second you on using more checks and fixing the code base. I had this stuff added to Travis too, but this was removed. It has not been introduced into the regular build cycle, too.

I hear the arguments of @pdurbin and others and understand their fear about not being able to use git blame easily. It's a real bummer: I really can't think of a way how to avoid that in Git.

[oscardssmith]

one answer would be to make separate patches for comon contributors that touch the lines they last committed, and have them merge their prices. it would be a pain, but would preserve the info of git blame

[pdurbin]

@pkiraly @poikilotherm The thing about continuous integration and Travis in particular is that as a team we don't look at https://travis-ci.org/IQSS/dataverse very often. A git hook preventing developers from pushing code would be effective but I don't think we want that.

@oscardssmith sorry by "git blame" I'm talking more about logical changes than people.

[pkiraly]

@pdurbin I agree that code style check should not be a show stopper. If the community don't use Travis output very often, then we don't have to integrate it that way. If there is some tool, which behaves like coverall.io - test coverage report triggered automatically by Travis displayed in a separate site, which is linked from the Github's repo main page, then it would be worth to intergate such a hook into Travis. I am not aware such a tool. For me simple command line check and create report locally works well.

@oscardssmith Making patches per contributors would be a good idea, but I don't really know how to implement it. We should separate the output of checkstyle by the ID of the last commiter of each source file. Sounds a little bit tricky, but not impossible nevertheless.