Fix formating of files accoring to precommit setup

[destogl]

Here is the result of the new format. We don't have to merge this, but it makes the consequences visible.

The whole reformatting took less than 10s on the whole repository.

[Karsten1987]

Sorry I don't think we can merge this PR.

First off, I'd love to see the CI changes separately from the actual code formatting. I think we should change CI and come up with code formatting in a second step.

However, looking at the diff, there's a couple of things I don't agree with.

• The clang format does not seem to comply with the rest of the ROS2 code standard. { are on a new line only on function definition, not after if's etc.
• pep (python formatting) recommends a single stroke ' for strings.
• Did you actively disable uncrustify here?

[bmagyar]

@Karsten1987 this PR is only for visibility, what you outlined is how it was done on the demos repo 👍

Here's my take on this:

• I really prefer wrapping braces everywhere, really helps reading the code and thinking about scopes. We don't hold any quality level ATM and even then level 2 only specifies "Must have a code style and enforce it".
• I agree, probably an easy fix.
• clang-format doesn't agree with uncrustify, one has to go
• Overall roughly -200 lines despite wrapping a lot of braces, I don't know where that comes from but very welcome

[Karsten1987]

I just generally not a fan of deviating from the rest of the ROS2 style guide and check/linters. If we settle on a clang-format I'd at least use the upstream clang format and look into clang tidy respectively.

The rest becomes a matter of personal taste, I for my side really appreciate not having these wrapping braces everywhere.

[destogl]

Sorry I don't think we can merge this PR.

First off, I'd love to see the CI changes separately from the actual code formatting. I think we should change CI and come up with code formatting in a second step.

Please see #394...

However, looking at the diff, there's a couple of things I don't agree with.

* The clang format does not seem to comply with the rest of the ROS2 code standard. `{` are on a new line only on function definition, not after `if`'s etc.

* pep (python formatting) recommends a single stroke `'` for strings.

I actually do not agree on this. I am using many integrated linters in other projects and there is rather a tendency to go for ". Also, my integrated PEP8 formatter is very much OK with this. This is also how it is done by black formatter since this is not explicitly regulated.

* Did you actively disable `uncrustify` here?

Yes, favoring clang-format.

[destogl]

* Overall roughly -200 lines despite wrapping a lot of braces, I don't know where that comes from but very welcome

clang-format is better in using line lengths than uncrustify.

[destogl]

I just generally not a fan of deviating from the rest of the ROS2 style guide and check/linters. If we settle on a clang-format I'd at least use the upstream clang format and look into clang tidy respectively.

We are using the same setup as in ament_clang_format package.

[Karsten1987]

The real show-stopper for me here is that you removed the functionality from the package.xml and CMakeLists.txt. This doesn't allow for an easy integration in existing CI setups.
Let's say my CI infrastructure relies on the ROS2 buildfarm or some derivative from it, how do I test my code?

[destogl]

The real show-stopper for me here is that you removed the functionality from the package.xml and CMakeLists.txt. This doesn't allow for an easy integration in existing CI setups.

I am not sure what you are pointing at. I only removed "linter" checks from "testing" stage. The reason for this is separation of linting and testing on the CI. If we keep this, we will have a collision between clang-format and uncrustify. This also makes the error search easier because you don't have to search for errors in build stages when there is a linting error.

Let's say my CI infrastructure relies on the ROS2 buildfarm or some derivative from it, how do I test my code?

In this case, you use a different setup. CI-setup should anyway be used per-repository because you have to at least write names of your ROS-packages. On the other hand everything is tested, but in different stages. From my experience this separation leads to clarity and focus when committing the code.

[bmagyar]

black --skip-string-normalization doesn't replace " with ', if that is a preference. I don't have a strong opinion on this but here you can find a thread where a lot of people do: psf/black#118

[Karsten1987]

@destogl I am referring to your changes in the package.xml and CMakeLists.txt.

if(BUILD_TESTING)
endif()

With this change, I am unable to integrate the ros2_control packages in existing ROS2 CI infrastructure as colcon test will simply not yield anything. With this, I have to set up extra infrastructure to detect linter failures.

I am okay if we want to introduce a clang-format here, all I am rooting for is that existing CI infrastructure will still co-exist and work as such (cppcheck, cpplint, pyflake, pep, etc.)
We can simply disable uncrustify with a simple command in cmake. As for the python formats, I'd really prefer to keep the existing ROS2 style (pep8, pep257 settings) to not deviate too much. When looking at code for the rest of ros2cli (ros2 topic, ros2 node, etc.) I don't see a reason why ros2 control should use a different code style.

[destogl]

@destogl I am referring to your changes in the package.xml and CMakeLists.txt.

if(BUILD_TESTING)
endif()

With this change, I am unable to integrate the ros2_control packages in existing ROS2 CI infrastructure as colcon test will simply not yield anything. With this, I have to set up extra infrastructure to detect linter failures.

I know that, and I tried to explain exactly that. Because I don't think that colcon test should test linters at all. Also, because it's output is not very useful, you have anyway than cat some test result file or run ament_* manually.
Actually, ROS2 provide separate workflow for the linters and this one is still configured and working here. So, I don't see how we deviate from it? Also, why do you want to test linters on the upstream packages in your private CI infrastructure?

My point is that using linters in tests also breaks normal builds with industrial_ci or those ros-tooling actions and this should not be the case. Because then we always have to look into logs, scroll until the end of the world to see that some linter failed, which can also simply see in another workflow.

We can simply disable uncrustify with a simple command in cmake. As for the python formats, I'd really prefer to keep the existing ROS2 style (pep8, pep257 settings) to not deviate too much. When looking at code for the rest of ros2cli (ros2 topic, ros2 node, etc.) I don't see a reason why ros2 control should use a different code style.

If it has to be OK, IMHO the code with " looks more modern, and it provides the same syntax and output as in C++. Using ' provides mixed outputs, e.g. 'parameter_name' not set here (c++) and "parameter_name" not set here (Python). I
find this more disturbing toward the users than a few changes in the code. Especially, you can write the code in the style you want and black will reformat it before you add a commit.

[Karsten1987]

Because I don't think that colcon test should test linters at all

That's a personal opinion, there's a complete opposite opinion in which a build should be marked as failed even when a linter failed. That's how we implicitly deal with it at ROS2 core packages, in which a build is only marked as green when there's no warning, all tests passed and no linter warnings occur.

Also, why do you want to test linters on the upstream packages in your private CI infrastructure?

I guess I still couldn't make my point really clear here. Let's say I have 20 packages in my CI + ros2_control or eventually want to test ros2_control on the ROS2 buildfarm. The 20 packages use colcon test as usual (e.g. they were created with ros2 pkg create), then 20 packages are correctly tested and linter'ed. But for ros2 control I have to use github precommits to verify that cppcheck and such are run and don't detect any errors. That's pretty inconvenient and at this point pretty unfeasible for me personally. Again, I don't have anything to say against GH precommits, if they work in conjunction with GH action here, that's awesome. But we have to maintain colcon test as it is right now. As mentioned before, we can disable uncrustify from being run, even though I feel with ament_uncrustify --reformat . one isn't much more in pain than calling clang-format.

I find this more disturbing toward the users than a few changes in the code

Again, personal opinion. I'd consider sticking to a standard which was chosen as a ROS2 default is reason enough to not deviate from it.

Especially, you can write the code in the style you want and black will reformat it before you add a commit.

This is great, and clang-format/ament_uncrustify --reformat can do a lot of work, but that's not the real pain point here. Things like pep, cppcheck, cpplint don't only format code, they validate your code. If there're tools for it which address all these things automatically that would be great - But I am not aware of such a tool.

[destogl]

I guess I still couldn't make my point really clear here. Let's say I have 20 packages in my CI + ros2_control or eventually want to test ros2_control on the ROS2 buildfarm. The 20 packages use colcon test as usual (e.g. they were created with ros2 pkg create), then 20 packages are correctly tested and linter'ed. But for ros2 control I have to use github precommits to verify that cppcheck and such are run and don't detect any errors. That's pretty inconvenient and at this point pretty unfeasible for me personally. Again, I don't have anything to say against GH precommits, if they work in conjunction with GH action here, that's awesome. But we have to maintain colcon test as it is right now. As mentioned before, we can disable uncrustify from being run, even though I feel with ament_uncrustify --reformat . one isn't much more in pain than calling clang-format.

I still don't get it. How can issue happen if everything in master branch is checked anyway. We don't remove any tests, but rather add additional ones here. Sorry, I cann't really see the issue here....

This is great, and clang-format/ament_uncrustify --reformat can do a lot of work, but that's not the real pain point here. Things like pep, cppcheck, cpplint don't only format code, they validate your code. If there're tools for it which address all these things automatically that would be great - But I am not aware of such a tool.

Black is actually addressing most of the pep issued automatically, so a reason more to keep it.

P.S. This is fun, but we will not come to a consensus I believe...