[gdal-dev] Automating code style enforcement ?

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

[gdal-dev] Automating code style enforcement ?

Even Rouault-2

Hi,

 

I've observed Kurt's continued efforts, over several months if not years, to set a consistant style to the code base and this is a much appreciated initiative. Now, as this touches the whole code base, I think this should be formalized, probably under the form of a RFC, so that this is shared among all developers. Currently as there's no automation, basically every committer will probably unconsciously "break" files that have been already reformatted, which is a waste of time that could be avoided (and on a minor note we could save extra reformatting commits that are noise in the history, and extra bits to parse for the human preparing a changelog)

 

I've seen in other projects, like QGIS, a script (in QGIS, they rely on astyle ultimately. not saying we should necessary use astyle) that takes care of reformatting automatically the files to be committed given the rules. That way the effort is spread on all contributors (and it is not a big deal to run this script). We could also have in Travis-CI a check so that pull requests are verified to respect the style (currently, we have already a very adhoc scripts/detect_tabulations.sh to check that no tabulation characters are used)

 

Even

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com


_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: Automating code style enforcement ?

Kurt Schwehr-2
Mateusz just created https://trac.osgeo.org/gdal/wiki/rfc69_cplusplus_formatting and I'll be cloning his https://trac.osgeo.org/geos/wiki/RFC4 with changes for GDAL.

On Thu, May 4, 2017 at 1:49 PM, Even Rouault <[hidden email]> wrote:

Hi,

 

I've observed Kurt's continued efforts, over several months if not years, to set a consistant style to the code base and this is a much appreciated initiative. Now, as this touches the whole code base, I think this should be formalized, probably under the form of a RFC, so that this is shared among all developers. Currently as there's no automation, basically every committer will probably unconsciously "break" files that have been already reformatted, which is a waste of time that could be avoided (and on a minor note we could save extra reformatting commits that are noise in the history, and extra bits to parse for the human preparing a changelog)

 

I've seen in other projects, like QGIS, a script (in QGIS, they rely on astyle ultimately. not saying we should necessary use astyle) that takes care of reformatting automatically the files to be committed given the rules. That way the effort is spread on all contributors (and it is not a big deal to run this script). We could also have in Travis-CI a check so that pull requests are verified to respect the style (currently, we have already a very adhoc scripts/detect_tabulations.sh to check that no tabulation characters are used)

 

Even

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com


_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev



--

_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: Automating code style enforcement ?

Mateusz Loskot
In reply to this post by Even Rouault-2
On 4 May 2017 at 22:49, Even Rouault <[hidden email]> wrote:
> Hi,
>
> I've observed Kurt's continued efforts, over several months if not years, to
> set a consistant style to the code base and this is a much appreciated
> initiative. Now, as this touches the whole code base, I think this should be
> formalized, probably under the form of a RFC, so that this is shared among
> all developers.

Even,

Yes, it is good time.
I've encouraged Kurt to take a step towards formal motion and
we are working on that.

> I've seen in other projects, like QGIS, a script (in QGIS, they rely on
> astyle ultimately. not saying we should necessary use astyle)

Please, let's forget about astyle.

> that takes care of reformatting automatically the files to be committed given the
> rules. That way the effort is spread on all contributors (and it is not a
> big deal to run this script). We could also have in Travis-CI a check so
> that pull requests are verified to respect the style (currently, we have
> already a very adhoc scripts/detect_tabulations.sh to check that no
> tabulation characters are used)

FYI, I prepared GEOS <https://trac.osgeo.org/geos/wiki/RFC4> proposal,
in major part, based on the MongoDB story.
There are references linked with details on
how MongoDB implemented such automated utilities.

Best regards,
--
Mateusz Loskot, http://mateusz.loskot.net
_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: Automating code style enforcement ?

Even Rouault-2

Hi Mateusz,

 

> Please, let's forget about astyle.

 

Because you consider it more limited feature-wise than clang-format ?

(https://trac.osgeo.org/geos/wiki/RFC4 : "wrapping long lines whether of code, strings, arrays - something which AStyle has no way of doing")

 

I'm wondering what you mean by "No automation of code reformatting is proposed. It would be treating the symptomps, no cause: developers not following the code formatting standard. "

 

I'd really want a script to fix edited files and fix them. From a quick look, it isn't obvious that my favorite text editor (kate) could ingest a .clang-format file to enforce the style when I'm editing. And honestly I don't even want to learn the rules of the style we would apply. 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.

 

Even

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com


_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: Automating code style enforcement ?

Mateusz Loskot
On 5 May 2017 at 09:22, Even Rouault <[hidden email]> wrote:
>> Please, let's forget about astyle.
>
> Because you consider it more limited feature-wise than clang-format ?

Because, clang-format is based on the same tooling as LLVM/clang,
at least the same tokenizer/lexer, so it can process complex and
convoluted C++ constructs within context; whenever
those tools are applied to LLVM or clang compiler itself,
libformat/clang-format update too.

> I'm wondering what you mean by "No automation of code reformatting is
> proposed. It would be treating the symptomps, no cause: developers not
> following the code formatting standard. "

Please, keep in mind that this draft was prepared and proposed for GEOS.
Kurt has copied it and, AFAIK, is going to review it applying changes
required by GDAL, based on feedback.

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.
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.

Automated styling of commits would just allow developers to ignore style
completely and commit any garbage with hope the magic will happen automatically.
That would be what is called treating the symptoms.


> I'd really want a script to fix edited files and fix them. From a quick
> look, it isn't obvious that my favorite text editor (kate) could ingest a
> .clang-format file to enforce the style when I'm editing.

Any change you are confusing .editorconfig?

> 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.

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.

> 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
or even running clang-format.

I'm fairly certain there is clang-format integration for Kate-based editors
that allow to run clang-format with shortcut.

Anyhow, I only meant to explain points of my original proposal for GEOS.

Best regards,
--
Mateusz Loskot, http://mateusz.loskot.net
_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: Automating code style enforcement ?

Even Rouault-2

> 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 ?

 

> 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.

 

>

> Automated styling of commits would just allow developers to ignore style

> completely and commit any garbage with hope the magic will happen

> automatically. That would be what is called treating the symptoms.

>

> > I'd really want a script to fix edited files and fix them. From a quick

> > look, it isn't obvious that my favorite text editor (kate) could ingest a

> > .clang-format file to enforce the style when I'm editing.

>

> Any change you are confusing .editorconfig?

 

Perhaps. I'm not familiar with it either.

 

>

> > 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)

 

>

> 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.

 

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. 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.

 

>

> > 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.

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com


_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: Automating code style enforcement ?

Sebastiaan Couwenberg
On 05/05/2017 01:11 PM, Even Rouault wrote:
> 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 ?

There is git2cl, but that only supports the GNU ChangeLog format.

IIRC git-cl is part of clang-format, but I don't think that's included
in the Debian package.

Kind Regards,

Bas

--
 GPG Key ID: 4096R/6750F10AE88D4AF1
Fingerprint: 8182 DE41 7056 408D 6146  50D1 6750 F10A E88D 4AF1
_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: Automating code style enforcement ?

Even Rouault-2
In reply to this post by Mateusz Loskot

On vendredi 5 mai 2017 10:45:33 CEST Mateusz Loskot wrote:

> On 5 May 2017 at 09:22, Even Rouault <[hidden email]> wrote:

> >> Please, let's forget about astyle.

> >

> > Because you consider it more limited feature-wise than clang-format ?

>

> Because, clang-format is based on the same tooling as LLVM/clang,

> at least the same tokenizer/lexer, so it can process complex and

> convoluted C++ constructs within context; whenever

> those tools are applied to LLVM or clang compiler itself,

> libformat/clang-format update too.

>

 

In case we want to have a policy regarding braces around single statements, one finding coming from the openjpeg discussion I mentionned :

"""

Moreover, concerning braces around single statements, it seems astyle does this with the add-braces option: http://astyle.sourceforge.net/astyle.html#_add-braces
As far as I understood, clang-format does not, and requires another tool (clang-tidy) to deal with these cases. So one more point for astyle.

"""

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com


_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: Automating code style enforcement ?

Mateusz Loskot
In reply to this post by Even Rouault-2
On 5 May 2017 at 13:11, Even Rouault <[hidden email]> 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
_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: Automating code style enforcement ?

Mateusz Loskot
In reply to this post by Even Rouault-2
On 5 May 2017 at 13:35, Even Rouault <[hidden email]> wrote:

>
> In case we want to have a policy regarding braces around single statements,
> one finding coming from the openjpeg discussion I mentionned :
>
> """
> Moreover, concerning braces around single statements, it seems astyle does
> this with the add-braces option:
> http://astyle.sourceforge.net/astyle.html#_add-braces
> As far as I understood, clang-format does not, and requires another tool
> (clang-tidy) to deal with these cases. So one more point for astyle.
> """

Rather, it is a separation of concerns: linting vs formatting
AFAIU, clang-format does not modify any code (ie. add tokens).

I'm not clang-format advocate. I'll repeat myself:
My personal preference is to use clang-format.
I proposed it to GEOS via GEOS RFC.
Kurt found the RFC useful and is using it as a template
to prepare one for GDAL.
IMHO, at the end of the day, tool does not matter as long as proposed
goals are achieved.

Best regards,
--
Mateusz Loskot, http://mateusz.loskot.net
_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: Automating code style enforcement ?

Kurt Schwehr-2
astyle would certainly work too.

I find clang-format more robust than almost all other formatting tools as Mateusz described.  As for workflow, there are multiple possible routes as to how to go about this.  I personally write pretty messy code when I'm working and then just do a format pass before submitting.  If the editor I'm using supports formatting, great.  However, the command line is super easy.  I also know a number of people who work on code only after applying their own person clang format style to code and then transform back before submitting.  And there is the critical ability to shut off the formatter for sections of code where the formatter will do something seriously wrong... the classic example is matrices in test files. 

Alternatively, we could easily have astyle as a first pass to add braces before clang-format if someone wanted to implement that.  But there clang-tidy as Mateusz said with:


with lots of other check....


On Fri, May 5, 2017 at 4:46 AM, Mateusz Loskot <[hidden email]> wrote:
On 5 May 2017 at 13:35, Even Rouault <[hidden email]> wrote:
>
> In case we want to have a policy regarding braces around single statements,
> one finding coming from the openjpeg discussion I mentionned :
>
> """
> Moreover, concerning braces around single statements, it seems astyle does
> this with the add-braces option:
> http://astyle.sourceforge.net/astyle.html#_add-braces
> As far as I understood, clang-format does not, and requires another tool
> (clang-tidy) to deal with these cases. So one more point for astyle.
> """

Rather, it is a separation of concerns: linting vs formatting
AFAIU, clang-format does not modify any code (ie. add tokens).

I'm not clang-format advocate. I'll repeat myself:
My personal preference is to use clang-format.
I proposed it to GEOS via GEOS RFC.
Kurt found the RFC useful and is using it as a template
to prepare one for GDAL.
IMHO, at the end of the day, tool does not matter as long as proposed
goals are achieved.

Best regards,
--
Mateusz Loskot, http://mateusz.loskot.net
_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev



--

_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: Automating code style enforcement ?

flippmoke
All,

Just wanted to contribute some experiences we have gained at Mapbox while doing our C++ work on attempting to work towards a common format on some libraries. We have found that clang format is very helpful, but it is still also very good if you have written documentation on how to follow the style at some location. RFC seems to be the right track to gather thoughts.

The best way we have found so far to use clang format has been to check in a ".clang-format" file with each repository. Once this is done, we put in a script in our makefile to automate the updating process https://github.com/mapbox/wagyu/blob/master/Makefile#L95-L96 as an example.

A couple thing we have realized:

- Even experienced with a style often make mistakes as we are often shifting between projects (as they have different styles)
- A `make indent` (clang format) is a simple way to fix problems and check them in on occasion (such as after big changes or before releases)
- Optimize the style for reading now for writing of the code
- If you want to be really strict you can have style checked as part of testing (I am not personally a huge fan of this). 

Thanks,

Blake Thompson


On Fri, May 5, 2017 at 1:04 PM, Kurt Schwehr <[hidden email]> wrote:
astyle would certainly work too.

I find clang-format more robust than almost all other formatting tools as Mateusz described.  As for workflow, there are multiple possible routes as to how to go about this.  I personally write pretty messy code when I'm working and then just do a format pass before submitting.  If the editor I'm using supports formatting, great.  However, the command line is super easy.  I also know a number of people who work on code only after applying their own person clang format style to code and then transform back before submitting.  And there is the critical ability to shut off the formatter for sections of code where the formatter will do something seriously wrong... the classic example is matrices in test files. 

Alternatively, we could easily have astyle as a first pass to add braces before clang-format if someone wanted to implement that.  But there clang-tidy as Mateusz said with:


with lots of other check....


On Fri, May 5, 2017 at 4:46 AM, Mateusz Loskot <[hidden email]> wrote:
On 5 May 2017 at 13:35, Even Rouault <[hidden email]> wrote:
>
> In case we want to have a policy regarding braces around single statements,
> one finding coming from the openjpeg discussion I mentionned :
>
> """
> Moreover, concerning braces around single statements, it seems astyle does
> this with the add-braces option:
> http://astyle.sourceforge.net/astyle.html#_add-braces
> As far as I understood, clang-format does not, and requires another tool
> (clang-tidy) to deal with these cases. So one more point for astyle.
> """

Rather, it is a separation of concerns: linting vs formatting
AFAIU, clang-format does not modify any code (ie. add tokens).

I'm not clang-format advocate. I'll repeat myself:
My personal preference is to use clang-format.
I proposed it to GEOS via GEOS RFC.
Kurt found the RFC useful and is using it as a template
to prepare one for GDAL.
IMHO, at the end of the day, tool does not matter as long as proposed
goals are achieved.

Best regards,
--
Mateusz Loskot, http://mateusz.loskot.net
_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev



--

_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev


_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev