Andrea,
I like pull requests that reformat lines close to changes, or even a
whole block. I especially like tab eradication. I agree with your
rejection of pull requests that make wholesale formatting changes
unrelated to the substantive changes.
I am -0 on a mass reformatting and also -0 on automated format checking,
but I am willing to listen to proposals. I remember the horrible
problems we had with Jalopy back in the olden days; perhaps checking on
Jenkins CI will be better?
The only "sacred hand formatting" I do is for foreign language multiline
string literals (for example, WKT, XML, and SQL, mainly in unit tests)
and the occasional long array initialiser where items need comments.
Java lacks multiline string literals, so we use concatenated strings
(Python """ where are you?). To preserve formatting inside the
concatenation of string literals, I nowadays use the @formatter tag
provided in our GeoTools Eclipse formatter:
// @formatter:off
// @formatter:on
There are several instances of @formatter use in GeoTools and GeoServer.
Try:
find . -name "*.java" -exec grep -H '@formatter:' {} \;
Does CheckStyle recognise Eclipse @formatter tags?
I am also guilty of, in the past, using trailing // comments to defeat
the Eclipse formatter. Using @formatter tags is much more sustainable.
Kind regards,
Ben.
On 11/02/17 23:47, Andrea Aime wrote:
> Hi,
> I'm reviving the code formatting dead horse (poor beast, beaten so many
> times) to see if we can
> improve and stop getting weekly complaints about the current state of code
> formatting
>
> So I think everybody agrees that properly formatted code is nice and
> readable, however we
> have had a number of pushbacks against it in the years for a variety of
> reasons:
>
> 1. Making it harder than necessary to review pull request
> 2. Making it harder to check history about a certain line of code (a
> reformat makes it hard run "blame" on the code, one has to find the
> reformat and then run blame against the revision before it)
> 3. Some coders trying to "table align" code and complaining against
> automatic reformats
>
> My take on it:
>
> 1. Pull request should not have reformats, period
> 2. Periodic reformarts would make it just too hard to maintain code, but
> I would not mind too much a single giant "whole world" reformat
> 3. I don't think we have anymore developers doing "sacred hand
> formatting" so not a problem I believe
>
> I've seen that in QGIS a formatting violation breaks the build. They have
> it good because they are using the same tool (astyle) for both checking the
> formatting and to reformat the code, inside the build, IDE independent.
> In our case for as much as we try, I don't think that we can make all IDE
> agree exactly on the same formatting, so I would still ban wholesale file
> reformatting during normal development.
>
> But we could maybe do a mass reformat on all active branches. There is a
> plugin here doing that based on a Eclipse config file (our coding
> conventions):
> http://code.revelc.net/formatter-maven-plugin/formatter-maven-plugin/examples.html#Setting_Source_Files
>
> We could do that, and be happy with the result... the formatting will then
> deteriorate over time again.
> Or we could add check for complaince... the plugin above seems to be
> supporting validation too (it might be a bit rigid and Eclipse specific
> though, need to try it out),
> or we could use a CheckStyle check.
> In both cases I'd be worried about the extra build time, and would like to
> verify if the overhead is acceptable or not.
>
> Or we can just leave things as is and just pay attention to formatting of
> new code in pull requests.
>
> Anyways... comments?
>
> Cheers
> Andrea
>
>
>
>
>
>
>
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, SlashDot.org! http://sdm.link/slashdot
>
>
>
> _______________________________________________
> GeoTools-Devel mailing list
> GeoTools-Devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/geotools-devel
>
--
Ben Caradoc-Davies <b...@transient.nz>
Director
Transient Software Limited <http://transient.nz/>
New Zealand
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
GeoTools-Devel mailing list
GeoTools-Devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geotools-devel
