On 03/17/2015 01:35 PM, Adam Taft wrote:
Whitespace is a holy war and should be avoided. Concern over two space or
four space indents, tabs, etc. is not relevant and adds no value (concern
with spacing is a form of muda). Diffs and other change evaluations can
ideally be done without care for whitespace. This is an easy option for
most diff tools.
The important thing is that we set a standard and stick to it. Workflows
commonly rebase on another branch or merge branches with one another,
which is a gigantic pain if there are needless conflicts because of
whitespace. And the bigger issue is leaking non-functional changes. A
few aren't a terribly big deal, but import reordering and changing
indentation are bad.
I'm also thinking about what I do for other projects to maintain a
consistent version when testing with other projects. In our
distribution, we snapshot versions and backport changes primarily with
cherry-picking. If those cherry-picks fail and have huge conflicts all
because of whitespace or import reordering, we have to do much more work
and our confidence is lower that we got the backport done right, without
bugs.
Sun + 4 space indents seems to be the "norm" for NiFi. I like Sean's
second proposal for using jenkins as a nag toward more consistent
formatting. There are code formatting options in each IDE to help as well.
+1 to nagging as soon as possible
But ultimately, I don't think a lot of effort should be spent on this.
Developers should just be mindful, as with any other type of contribution,
that they should conform to the lay of the land.
I agree in principal, but the way to make this happen is to set a
standard and stick to it. Calling each other on this in code reviews or
automatically will ensure the standard is followed so we can all stop
thinking about it.
I personally prefer tabs over spaces for my indents (I could rant why this
is "correct" -- again, holy war). But if I'm contributing to NiFi, I'm
using 4 space indents, because that's what is normal for the project.
Two cents.
Adam
On Tue, Mar 17, 2015 at 2:29 PM, Sean Busbey <[email protected]> wrote:
Set a coding standard (sun + 2 space indents is common), then use
checkstyle to complain when things don't match. It helps when you can
provide IDE formatting configurations that match your chosen coding
standard.
For what it's worth, this is what I see on most of the Apache projects
that I work on. It is slightly more economical if we introduce a rule
about line maximums.
And speaking of line limits, I think that's one where we don't want to
have automated enforcement. There's a good argument for it (for
vertically split editors) but I'd rather a line go a little long because
of a change than have that change affect the lines around it. Developer
discretion there.
Some of the bigger projects use a jenkins precommit bot that checks (among
other things) that the checkstyle violations don't go up with a proposed
patch. The bot comments on the jira and the committers decide if any
violations need to get fixed. Personally, I like this approach better than
e.g. trying to fail the build with checkstyle because it allows incremental
improvement.
rb
--
Ryan Blue
Software Engineer
Cloudera, Inc.
