Collectible Card Game Headquarters

We are having problems with checkstyle.
Some developers are commiting other people's files after applying autoformatter to them.

What I would like to suggest - accept a coding convention (if there is none yet) and set up all the autoformatting tools across all developers' systems to produce code styled in the same way. This way, I hope, we'll avoid similiar problems in future.

Max mtg wrote:Hi,
I've just updated my code the latest version from the repository. I'm always wondering what others have done and learn that by comparing newcoming versions with old working copies I had. Once there are a few lines of diffs - it's quite easy to understand, but it's much-much harder when you see the whole file coloured because the one who commited the file applied autoformatting to it.

Especially I refer to this case:

Code: Select all

http://svn.slightlymagic.net/websvn/comp.php?compare[]=%2Ftrunk%2Fsrc%2Fmain%2Fjava%2Fforge%2Fcard%2FcardFactory%2FCardFactoryUtil.java%409782&compare[]=%2Ftrunk%2Fsrc%2Fmain%2Fjava%2Fforge%2Fcard%2FcardFactory%2FCardFactoryUtil.java%409748

Can anyone see which changes are relevant to committed patch-note and which are due to autoformatter tool?

click Ignore Whitespace in the upper right.

typically, it is a good idea to ignore changes in the amounts of whitespace in all code comparisons. for example, if you use the command line, use {diff -b} and {svn diff -x -b}. if you don't, you have to find the setting in your diff gui. ignoring all whitespace is second best.

there are other problems with WebSVN at present. you can use Subclipse or another Subversion tool to view better diff output.

if you use Eclipse with the Checkstyle plugin, it will issue errors for all style violations for each .java and .properties file you have open. do you have the instructions from the wiki with the table of recommended plugins for Eclipse?

Max mtg wrote:What I would like to suggest - accept a coding convention (if there is none yet) and set up all the autoformatting tools across all developers' systems to produce code styled in the same way. This way, I hope, we'll avoid similiar problems in future.

we use Checkstyle as the coding convention. also, i think the project-specific Eclipse formatting standards already enforce no tabs, proper left curly brace placement, and no wildcard star imports. for , press {Ctrl+Shift+O} {that's a letter "Oh"} to organize all of the imports in the current file you're editing.

many files have violations. we fix them slowly. personally, i try to address at least some Checkstyle, FindBugs, and PMD messages in files where i have changed a significant amount of code. these are the files i will also be testing the most rigorously before committing.

but Forge is somewhat fragile, so there is often danger in changing too much, even if it seems innocuous. once I changed a (new Integer(x)) to an int, because Java 5 does automatic casts between these types. it compiled fine, but it broke the GUI. i still do not understand why. it was very strange.

i have read the file again. i see now that ignoring whitespace only helps a little.

that code is not in compliance with our coding standards.

i think the moral of the story is that we would prefer massive style changes to happen in a separate commit.

You say Eclipse enforces proper left curly brace placements, but IME it seems a bit well.. all over the place.I've always gone with "opening brace alone on the next line from the block declaration(function/class/whatever),content on following lines 1 tab (8 spaces) in".Also, "Always curly brace block for if's even if the case is just one line.".

Either way, I'm going to keep a better eye on Checkstyle reports from now on.

Welp, a tiny contribution is a contribution at least.

EDIT: For clarity, I am not putting the foot down on this, just... eh, see my next post in the topic.

Did I miss the conversation where we discussed coding conventions? I don't remember any of that being decided on by more than one person declaring it to be so

If you're referring to me, I apologise if I came across that way. It just seems like the prevailing convention with odd outcrops of diffence in places, as well as being the way Checkstyle likes it.

Braids wrote:i have read the file again. i see now that ignoring whitespace only helps a little.

i think the moral of the story is that we would prefer massive style changes to happen in a separate commit.

Yes, that is the very problem.

That's exactly what I mean.
If one wishes to change the style that much, let it be two commits - one for the changes in code and another one for changes in style.

Hellfish wrote:If you're referring to me, I apologise if I came across that way. It just seems like the prevailing convention with odd outcrops of diffence in places, as well as being the way Checkstyle likes it.

wasn't you in particular. It just seemed like somewhere in all this svn and git juggling check style was declared the way to follow and I didn't hear any discussion regarding it at all

friarsol wrote:Did I miss the conversation where we discussed coding conventions? I don't remember any of that being decided on by more than one person declaring it to be so

there were hints in http://www.slightlymagic.net/forum/viewtopic.php?f=52&t=4759&p=65420&hilit=checkstyle#p65411 and a much more explicit topic at http://www.slightlymagic.net/forum/viewtopic.php?f=52&t=5062&hilit=checkstyle .

Braids wrote:i have read the file again. i see now that ignoring whitespace only helps a little.

i think the moral of the story is that we would prefer massive style changes to happen in a separate commit.

we = Max and I.

i also neglected to mention that i myself am guilty of bundling style in my commits. i intend to change my ways.

Braids wrote:i think the moral of the story is that we would prefer massive style changes to happen in a separate commit.

i also think that organizing the imports does not count as "massive style changes". these tend to be limited to a section of code that has little logic. or as pmd would call it, the cyclomatic complexity of the import section is zero.

Hellfish wrote:You say Eclipse enforces proper left curly brace placements, but IME it seems a bit well.. all over the place.I've always gone with "opening brace alone on the next line from the block declaration(function/class/whatever),content on following lines 1 tab (8 spaces) in".Also, "Always curly brace block for if's even if the case is just one line.".

CheckStyle is presently allowing left curly braces on one-liners. the following is legal.

Code: Select all

if (foo) {


For multi-line blocks, CheckStyle demands the brace be on its own line:

Code: Select all

class Proletariat implements Interface1, Interface2,
    Interface3
{


i'm not sure if CheckStyle will complain if you place your left curly brace on the next line for a one-liner.

CheckStyle rejects all tab characters. Eclipse is presently configured to indent each block by 4 spaces. i think the major reason for this is to make diff output consistent among all platforms.

CheckStyle presently requires curly braces for all blocks. the following is a violation.

Code: Select all

if (a) b;


it must instead be

Code: Select all

if (a) {
    b;
}


CheckStyle allows empty blocks, but it flags empty statements. using the example of an empty constructor, this is ok:

Code: Select all

    public Proletariat {  // constructor
    }


this is not ok:

Code: Select all

    public Proletariat ;


Hellfish wrote:EDIT: For clarity, I am not putting the foot down on this, just... eh, see my next post in the topic.

your next post in this topic did not seem particularly substantive. are you still drafting it?

Braids wrote:

friarsol wrote:Did I miss the conversation where we discussed coding conventions? I don't remember any of that being decided on by more than one person declaring it to be so

there were hints in http://www.slightlymagic.net/forum/viewtopic.php?f=52&t=4759&p=65420&hilit=checkstyle#p65411 and a much more explicit topic at http://www.slightlymagic.net/forum/viewtopic.php?f=52&t=5062&hilit=checkstyle .

Unfortunately, there isn't much of a discussion happening on that thread. Rob requests no one line conditional statements, Chris is curious about the reports and the rest is you talking about CheckStyle in a few different ways.

I'm not sure why things were added and why things weren't. I don't know why the styling I've been using for the last year is now "wrong". It seems we quickly went from "I'm enabling a bunch of stuff" to "CheckStyle says you are doing things incorrectly."

I can adjust certain aspects of my coding style to meet the "Decided Standard" but if I have no idea why that "Standard" came to be, or what it actually is, I'm not sure how I could follow it. A 100+ line CheckStyle prefs diff doesn't mean anything to someone who hasn't used the utility before. I agree that a consistent styling is a good thing, but I'd rather deal with different styling in the codebase for awareness of Project Wide changes such as this.

friarsol wrote:

Braids wrote:

friarsol wrote:Did I miss the conversation where we discussed coding conventions? I don't remember any of that being decided on by more than one person declaring it to be so

there were hints in {a couple of threads}.

Unfortunately, there isn't much of a discussion happening on that thread.

true enough. how about we take the time now to discuss it? i've learned how to configure CheckStyle, and i can make modifications. trust me, the original config was even more cruel. it was actually rather stupid; CheckStyle would complain that a constructor had an empty block. but upon removal of the superfluous constructor, it would complain that we should not use default constructors. that, and it sometimes conflicted with FindBugs or PMD.

friarsol wrote:I'm not sure why things were added and why things weren't. I don't know why the styling I've been using for the last year is now "wrong". It seems we quickly went from "I'm enabling a bunch of stuff" to "CheckStyle says you are doing things incorrectly."

i think "things" were added as part of maven's ability to generate reports about CheckStyle, FindBugs, and PMD and post them to a web site.

we've not had coding standards before, so when asked about them, CheckStyle was the only reference i have. it's obviously making you unhappy, so let's fix it.

are there particular things you like that CheckStyle does not?

friarsol wrote:I can adjust certain aspects of my coding style to meet the "Decided Standard" but if I have no idea why that "Standard" came to be, or what it actually is, I'm not sure how I could follow it. A 100+ line CheckStyle prefs diff doesn't mean anything to someone who hasn't used the utility before. I agree that a consistent styling is a good thing, but I'd rather deal with different styling in the codebase for awareness of Project Wide changes such as this.

actually, i think that prefs diff in the post was an eclipse config file.

i wasn't entirely certain how long it had been used on the project, but it was the only thing we had, so i have been running with it. i wasn't trying to say anyone's style is wrong or invalid. all i know is what CheckStyle doesn't like.

do you want to get rid of it entirely? i want this discussion to feel less dictatorial and more democratic.

Coding style is so personal, I don't think it should even be an issue. I don't like one line if's... but if the code works and I never have to read/change it, ultimately I don't care. From my C background, "{" goes on its own line... as I picked up Java through Forge, I saw more and more "if (something) {" so I started emulating that style.