[gdal-dev] Automating code style enforcement ?

[Mateusz Loskot]

On 5 May 2017 at 13:11, Even Rouault <even.rouault at spatialys.com> wrote:
>> I made (or copied and agreed with) the statement that the best way to
>> reliably maintain consistent code formatting is to learn how to write
>> properly formatted code and ensure (eg. via code review) all developers
>> learn it too.
>
>> Certainly, tools like `git cl format` would be very handy to let
>> developers
>> ensure their commits are properly formatted.
>
> For now, we are still on svn, so a relatively SCM-independant solution would
> be better.
> By the way, git cl format doesn't seem available out-of-the-box on my Ubuntu
> 16.04. Perhaps there's an additional package to install ?

As mentioned in RFC too, it is just an example of an integration
https://chromium.googlesource.com/chromium/src/+/master/docs/clang_format.md

Doesn't matter how one integrates the same or similar tool, shell command,
whatever.

>> Finally, CI build as safety valve is necessary to notify about any style
>> drifting away from the agreed convention.
>> A pre-commit hook rejecting incompatible styles are just a stricter
>> variation.
>
>
>
> Yes, but I'm not sure we want to depend on SVN pre-commit hooks, so as to be
> more future proof. I'd rather see it as a check as part as one of the
> Travis-CI builds.


Yes, indeed. I agree, that is why I suggested developers should become
somewhat familar
with the coding style and conventions, integrate necessary tools into their IDEs
and use it locally, before committing. Some minimal effort is unavoidable.

>> Any change you are confusing .editorconfig?
>
> Perhaps. I'm not familiar with it either.

No need to become.
My RFC gives an overview of .editorconfig but it is not necessary at all.
However, it might be convenient for some developers to automate
basic traits of text encoding like utf-8, indent with spaces only,
remove trailing whitespaces, etc.

.editorconfig might interfere with git eol settings though,
eg. GEOS decided to force LF for all sources in all development environments,
while git can handle it already, of course (see GitHub recommendations)
Unix use LF
Windows users stick to CRLF
and repo receives unified EOLs, eg. LF.

>> > And honestly I don't even want to learn the rules of the style we would
>> > apply.
>> As per the proposal I made for GEOS, as long as you don't forget to run
>> clang-format tool before committing, you don't need to learn.
>
> But you must remember to run it on every file you touched, which you can
> easily forget at some point. Some tiny wrapping on top of that to iterate
> over all modified files would be convenient (hint: what does QGIS'
> scripts/prepare-commit.sh)

AFAIU, it is local hook, then it is fine.
It fits in the local development environment integrations,
You can integrate, script out, automate however one likes.
clang-format is a command line tool. I see no difference to astyle
in terms of usability.

>> However, I do assume that becoming a project developer and committer,
>> one agrees to learn how to write code that is expected and accepted by
>> a project.
>
> As soon as you are involved in several projects, there will be different
> code styles applied, so don't expect people (at least me) to know the
> precise code style to apply for a particular one.

Sure, and I don't expect anyone memorising any rules, if a projects says:
we use clang-format, here is .clang-format config file, you are free to use
whatever you like to ensure your commits follow that config.

> Honestly I don't care about if spaces are put before or after parentheses
> (I'm pretty sure I'm not self-consistent regarding that. Depends on my mood
> of the day), or such details. Mostly consistant indentation is needed for
> readability.

I agree.
As long as code is consistent, particular style does not matter.

I proposed reformatting for GEOS because its source code
formatting is pretty bad and messy, and you can see several
styles, tabs/spaces and others mixture in a single file.

Although GDAL code is in good shape, Kurt has been applying corrections
wherever needed, so I suggested to simplify the task with clang-format.

> Other people are more sensitive to that, and I'm fine with
> that. But we should really make it easy to everybody to ultimately have a
> way to submit compliant code, either in their editor, or as a pre-commit
> action. The later is even more true for casual contributors.

I believe I have addressed that earlier.

>> > We *need*
>> > something to fix it easily like scripts/prepare-commit[.sh/.py] that
>> > will
>> > check svn/git edited files and run them through the reformatter.
>>
>> As I pointed in the RFC, such convenient tools are available:
>> git cl format from Chromium or the python script
>
> https://github.com/mongodb/mongo/blob/master/buildscripts/clang_format.py
> you mean ? Seems to be git only.

It is just an example.

Best regards,
-- 
Mateusz Loskot, http://mateusz.loskot.net