giraph-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Avery Ching (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (GIRAPH-231) Overly prescriptive check-style requirements considered harmful
Date Mon, 02 Jul 2012 22:52:57 GMT

    [ https://issues.apache.org/jira/browse/GIRAPH-231?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13405410#comment-13405410
] 

Avery Ching commented on GIRAPH-231:
------------------------------------

I think it's reasonable to discuss this and I don't have a strong feeling regarding the exact
choices (well, okay, the 80 character lines is nice to have).  The main thing I care about
is that its consistent and most of the contributors are happy with it.  Code uniformity is
important to me as it is confusing to have many different styles.  I don't necessarily think
that the checkstyle choices we have made are the "best" ones, but overall they are helping
to keep the code uniform, which I appreciate.

I just don't want to see
{noformat}
for(int i=0;i<0;++i) 
{noformat}
in one block and then
{noformat}
for (int i = 0; i < 0; ++i)
{noformat}
in another.  This makes the code look messy and confuses contributors (well, at least me).
                
> Overly prescriptive check-style requirements considered harmful
> ---------------------------------------------------------------
>
>                 Key: GIRAPH-231
>                 URL: https://issues.apache.org/jira/browse/GIRAPH-231
>             Project: Giraph
>          Issue Type: Bug
>            Reporter: Jakob Homan
>
> The current checkstyle settings are extremely precise and an excellent codification of
particular coding style preference.  However, like in religion and politics, reasonable and
thoughtful people can have disagreements and should be accommodating to each other.  The current
checkstyle requirements venture into a lot of territory where disagreements and common and
often conflict with other styles that are perfectly reasonable.  Right now one can generate
a perfectly reasonable looking patch that then takes longer to make checkstyle than it did
to create it.
> A few examples:
> * Whether or not a for or if statement has a space before its opening paren does not
in any way make the code less or more readable or bug free.  Either preference is valid. 

> * Not every method or field requires javadoc.  I trust every contributor (or, barring
that, reviewer) to use their experience and judgment to determine if one is needed
> * 80 characters per line is a reasonable arbitrary limit.  But so is 85 or 90.  And 80
seems to cause a lot of lines to be cut off at very odd places from a readability standpoint.
 I trust every contributor (or, barring that, reviewer) to determine if the line will cause
some huge system failure by going to 83 characters.  For instance which is worse:
> {noformat}
> String timeUnitString = conf.get(GIRAPH_METRICS_DEFAULT_TIME_PERIOD,
>     "SECONDS");
> {noformat}
> or
> {noformat}
> String timeUnitString = conf.get(GIRAPH_METRICS_DEFAULT_TIME_PERIOD, "SECONDS");
> {noformat}
> Everybody has a preference on each of the items above, but I don't think anyone can reasonably
make an argument that another's preference leads to more bugs or is objectively bad.
> Overly strict checkstyle settings, which is what I think we've ended up with, don't actually
end up improving the readability of the code.  Instead, they add a large amount of friction
between contributors.  If I spend most of my time in using another style that doesn't allow
spaces between if and (, I find it painful and frustrating to try to contribute to this project.
 Readability is not something that can be guaranteed by any checkstyle configuration and instead,
we should loosen the requirements and trust our contributors and reviewers to keep a good
eye out for subtle errors and leave checkstyle to police the egregious ones.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

Mime
View raw message