Code style

[Davide Giannella <gi...@gmail.com>]

Hello team,

I just spent some time in fixing formatting issues for a pull request ;)

Question: why don't we use the maven checkstyle plugin? In this way it
can check the styles (we could plug it in the pedantic profile) and it
could even fail the build if some exceptions occurred.

As an additional value, IIRC, it should be possible in eclipse (and I
guess IntelliJ) as well to create a formatter starting from the .xml
file that configure the checkstyle.

if there're no complains I'll file an issue about it and we'll implement
in our codebase as soon as we have time.

WDYT?

Cheers
Davide

Re: Code style

[Jukka Zitting <ju...@gmail.com>]

Hi,

On Tue, Mar 4, 2014 at 10:47 AM, Davide Giannella
<gi...@gmail.com> wrote:
> I'm saying that with the proper tool in place this would have not happened
> as the build would (potentially) break if the formatting is not good.

The trouble with such tooling is avoiding false positives. Where do
you draw the line on valid formatting?

For example many of the formatting changes in the troublesome patch
seem equally valid before and after the change. Which one should such
a tool prefer, and why?

Basic stuff like using spaces instead of tabs would probably be OK to
enforce in the default build, but already things like the correct
indentation level becomes troublesome to enforce automatically. There
are cases where it makes sense to break or bend the formatting rules
for better readability, and I'd hate to loose that flexibility because
of too strict formatting checks.

> In any case I could have a precise report of file name and line number
> of what is wrong and at the end even the committer should not care about
> the formatting as a tool is taking care of it by highlighting in case of
> problems.

When submitting a patch, it's always a good practice to actually take
a look at the diff and make sure that all the intended changes are
included and nothing else. Just a few seconds of looking at
https://github.com/apache/jackrabbit-oak/pull/9/files shows the
problem.

BR,

Jukka Zitting

Re: Code style

[Davide Giannella <gi...@gmail.com>]

Hi Jukka,

On 04/03/2014 15:12, Jukka Zitting wrote:
> Hi,
>
> On Tue, Mar 4, 2014 at 8:11 AM, Davide Giannella
> <gi...@gmail.com> wrote:
>> I raised the topic as the issue I'm working with (OAK-1263) is costing
>> me quite some time in fixing formatting.
> The formatting wasn't broken to begin with, it was your patch that was
> changing perfectly valid formatting.
>
> The problem here isn't our lack of formatting rules, but rather your
> overly eager IDE configuration. This wouldn't be such a problem if we
> all used the exact same IDEs with the exact same configuration and the
> IDEs made perfect formatting choises in all cases, but that's an
> impossible requirement especially for an open source project.
>
>> I know that for a committer this is not felt as you simply commit but if
>> you submit a patch then you have to go though the pain of fixing the
>> formatting according to "committer mood" rather than some written rules.
> The only reason you have that pain in the first place is that your IDE
> is configured to automatically apply formatting rules. Disable that
> and you only need to worry about the lines of code that you're
> actually changing.
I'm not saying that my patch didn't brake any formatting. That's why I'm
fixing as much as possible. Unfortuately with the change of the computer
some Eclipse settings went away and now the damage is done. :) I'm
saying that with the proper tool in place this would have not happened
as the build would (potentially) break if the formatting is not good.

In any case I could have a precise report of file name and line number
of what is wrong and at the end even the committer should not care about
the formatting as a tool is taking care of it by highlighting in case of
problems.

Cheers
Davide

Re: Code style

[Jukka Zitting <ju...@gmail.com>]

Hi,

On Tue, Mar 4, 2014 at 8:11 AM, Davide Giannella
<gi...@gmail.com> wrote:
> I raised the topic as the issue I'm working with (OAK-1263) is costing
> me quite some time in fixing formatting.

The formatting wasn't broken to begin with, it was your patch that was
changing perfectly valid formatting.

The problem here isn't our lack of formatting rules, but rather your
overly eager IDE configuration. This wouldn't be such a problem if we
all used the exact same IDEs with the exact same configuration and the
IDEs made perfect formatting choises in all cases, but that's an
impossible requirement especially for an open source project.

> I know that for a committer this is not felt as you simply commit but if
> you submit a patch then you have to go though the pain of fixing the
> formatting according to "committer mood" rather than some written rules.

The only reason you have that pain in the first place is that your IDE
is configured to automatically apply formatting rules. Disable that
and you only need to worry about the lines of code that you're
actually changing.

BR,

Jukka Zitting

Re: Code style

[Davide Giannella <gi...@gmail.com>]

On 04/03/2014 08:49, Thomas Mueller wrote:
> Sure, the exact formatting rules are a matter of personal taste. But why
> do companies such as Google *enforce* specific rules?
>
>     http://google-styleguide.googlecode.com/svn/trunk/javaguide.html
>
>
> Becase most of the time, developers *read* the code. And it's not just you
> who reads your code, but everybody else as well. Formatting rules help
> people read the code. I personally have a hard time understanding code
> that doesn't follow basic coding conventions. In fact I dislike such code.
> If you read a book, you expect certain formatting rules (identation,
> whitespace usage, and so on), and if those rules are broken, you can not
> concentrate on the content.
>
> In addition, some code conventions avoid bugs, for example the recent
> famous Apple SSL/TLS bug:
>
>     https://www.imperialviolet.org/2014/02/22/applebug.html
>
> With decent rules, and the Maven checkstyle plugin, it's easy to avoid
> such bugs.
>
> Therefore, I think we should use strict rules.
@Thomas, don't take it personally, I'm only using the Jira we're on as
an example of what I meant :)

I personally agree on code conventions and I was suggesting to use
checkstyle as it would ease our day-by-day work highlighting in time
what could be wrong and with some extra tools (Eclipse, etc) fix quickly
any formatting issues. On top there won't be any "battles" between we
use this or that as the rules in the checkstyle will say what is required.

I raised the topic as the issue I'm working with (OAK-1263) is costing
me quite some time in fixing formatting. Time that I could have spent in
developing new features. All things that if we have the maven plugin in
place it would have highlighted much earlier.

https://issues.apache.org/jira/browse/OAK-1263

I'm only trying to see if we could improve the process as I'm going
through the experience right now :)

So again I'm asking. Do we want to enforce any code conventions? If yes
we should use a tool to ease our job otherwise we should not bother at all.

For example the rules I got so far

* spaces only
* 4 spaces indentation
* spaces between statements (if (blblblala) {)
* No * import
* still some doubts on static imports vs non-static ones. Some told me
yes some told me no.

I know that for a committer this is not felt as you simply commit but if
you submit a patch then you have to go though the pain of fixing the
formatting according to "committer mood" rather than some written rules.

Peace and Love ;)
Davide

Re: Code style

[Jukka Zitting <ju...@gmail.com>]

Hi,

On Wed, Mar 12, 2014 at 7:09 AM, Julian Reschke <ju...@gmx.de> wrote:
> I like consisting coding style, but then, I also like to be able to violate
> a rule in some cases.

+1

> That being said: there are more things we should check, such as line ending
> (svn:eol-style), use of trailing whitespace, or non-ASCII characters in Java
> code.

Can we turn this conversation into a patch against the -Ppedantic
mode? I think that's the only good way to determine where the correct
level of strictness lies.

BR,

Jukka Zitting

Re: Code style

[Julian Reschke <ju...@gmx.de>]

On 2014-03-11 16:41, Felix Meschberger wrote:
> +1
>
> Coding style is part of the corporate identity of the code, in a sense.
>
> Regards
> Felix
> ...

I like consisting coding style, but then, I also like to be able to 
violate a rule in some cases.

That being said: there are more things we should check, such as line 
ending (svn:eol-style), use of trailing whitespace, or non-ASCII 
characters in Java code.

Best regards, Julian

Re: Code style

[Felix Meschberger <fm...@adobe.com>]

+1

Coding style is part of the corporate identity of the code, in a sense.

Regards
Felix

Am 04.03.2014 um 09:49 schrieb Thomas Mueller <mu...@adobe.com>:

> Hi,
> 
>>>> Question: why don't we use the maven checkstyle plugin?
>>> for what? we are quite liberal in coding style and respect personal
>>> preferences. the only checkstyle rules that make sense are the ones
>>> that find or prevent bugs (like avoiding .* imports).
> 
> Sure, the exact formatting rules are a matter of personal taste. But why
> do companies such as Google *enforce* specific rules?
> 
>    http://google-styleguide.googlecode.com/svn/trunk/javaguide.html
> 
> 
> Becase most of the time, developers *read* the code. And it's not just you
> who reads your code, but everybody else as well. Formatting rules help
> people read the code. I personally have a hard time understanding code
> that doesn't follow basic coding conventions. In fact I dislike such code.
> If you read a book, you expect certain formatting rules (identation,
> whitespace usage, and so on), and if those rules are broken, you can not
> concentrate on the content.
> 
> In addition, some code conventions avoid bugs, for example the recent
> famous Apple SSL/TLS bug:
> 
>    https://www.imperialviolet.org/2014/02/22/applebug.html
> 
> With decent rules, and the Maven checkstyle plugin, it's easy to avoid
> such bugs.
> 
> Therefore, I think we should use strict rules.
> 
> 
> Regards,
> Thomas
> 
> 
> 

Re: Code style

[Thomas Mueller <mu...@adobe.com>]

Hi,

>>>Question: why don't we use the maven checkstyle plugin?
>> for what? we are quite liberal in coding style and respect personal
>> preferences. the only checkstyle rules that make sense are the ones
>> that find or prevent bugs (like avoiding .* imports).

Sure, the exact formatting rules are a matter of personal taste. But why
do companies such as Google *enforce* specific rules?

    http://google-styleguide.googlecode.com/svn/trunk/javaguide.html


Becase most of the time, developers *read* the code. And it's not just you
who reads your code, but everybody else as well. Formatting rules help
people read the code. I personally have a hard time understanding code
that doesn't follow basic coding conventions. In fact I dislike such code.
If you read a book, you expect certain formatting rules (identation,
whitespace usage, and so on), and if those rules are broken, you can not
concentrate on the content.

In addition, some code conventions avoid bugs, for example the recent
famous Apple SSL/TLS bug:

    https://www.imperialviolet.org/2014/02/22/applebug.html

With decent rules, and the Maven checkstyle plugin, it's easy to avoid
such bugs.

Therefore, I think we should use strict rules.


Regards,
Thomas

 

Re: Code style

[Davide Giannella <gi...@gmail.com>]

On 03/03/2014 21:50, Jukka Zitting wrote:
> As a general rule, if you're changing existing code, stick with the
> existing formatting (i.e. no line in a path should be about
> formatting, unless the patch is explicitly about that).
>
> If introducing new code, we use the standard Java coding conventions
> (with spaces instead of tabs) as the main guideline, without
> specifying or policing the formatting in any more detail.
>

Got it! :)

D.

Re: Code style

[Jukka Zitting <ju...@gmail.com>]

Hi,

On Mon, Mar 3, 2014 at 12:20 PM, Tobias Bocanegra <tr...@apache.org> wrote:
> On Mon, Mar 3, 2014 at 6:14 AM, Davide Giannella
> <gi...@gmail.com> wrote:
>> Question: why don't we use the maven checkstyle plugin?
> for what? we are quite liberal in coding style and respect personal
> preferences. the only checkstyle rules that make sense are the ones
> that find or prevent bugs (like avoiding .* imports).

+1

As a general rule, if you're changing existing code, stick with the
existing formatting (i.e. no line in a path should be about
formatting, unless the patch is explicitly about that).

If introducing new code, we use the standard Java coding conventions
(with spaces instead of tabs) as the main guideline, without
specifying or policing the formatting in any more detail.

BR,

Jukka Zitting

Re: Code style

[Davide Giannella <gi...@gmail.com>]

On 03/03/2014 17:20, Tobias Bocanegra wrote:
> Hi,
>
> On Mon, Mar 3, 2014 at 6:14 AM, Davide Giannella
> <gi...@gmail.com> wrote:
>> I just spent some time in fixing formatting issues...
> what would that be?

Well, spaces, line wrapping, indentation, that changed on an existing
class and Eclipse that decided to apply some
rules.

>> ...for a pull request ;)
> we don't use git directly but svn - so I don't really know why a pull
> request would matter...

I know. But when submitting a patch to the ticket I also raise a pull
request so that it will be easier to see on github what changed for the
committer.

>> Question: why don't we use the maven checkstyle plugin?
> for what? we are quite liberal in coding style and respect personal
> preferences. the only checkstyle rules that make sense are the ones
> that find or prevent bugs (like avoiding .* imports).
>
>> In this way it
>> can check the styles (we could plug it in the pedantic profile) and it
>> could even fail the build if some exceptions occurred.
> we already have the RAT plugin for the license checks. what other
> styles you want to check or enforce?
Rat enforce the licence headers while the checkstyle automatically
checks for example that there're no tabs for indentation. You can extend
it as much as you like.

But got the message that there're no style preferences and that anyone
does almost what they want as long as they use spaces and respect what
was there before :)

Cheers
Davide

Re: Code style

[Tobias Bocanegra <tr...@apache.org>]

Hi,

On Mon, Mar 3, 2014 at 6:14 AM, Davide Giannella
<gi...@gmail.com> wrote:
> I just spent some time in fixing formatting issues...
what would that be?

> ...for a pull request ;)
we don't use git directly but svn - so I don't really know why a pull
request would matter...

> Question: why don't we use the maven checkstyle plugin?
for what? we are quite liberal in coding style and respect personal
preferences. the only checkstyle rules that make sense are the ones
that find or prevent bugs (like avoiding .* imports).

> In this way it
> can check the styles (we could plug it in the pedantic profile) and it
> could even fail the build if some exceptions occurred.
we already have the RAT plugin for the license checks. what other
styles you want to check or enforce?

> As an additional value, IIRC, it should be possible in eclipse (and I
> guess IntelliJ) as well to create a formatter starting from the .xml
> file that configure the checkstyle.
as I said - personal coding style has preference...

> if there're no complains I'll file an issue about it and we'll implement
> in our codebase as soon as we have time.
feel free to file an issue and propose your suggested checks.

regards, toby