Use negative indenting for access specifiers

[fbe555]

In order to get correct indentation of access specifiers in nested classes, the negative of the indent_columns value is used instead of the absolute leftmost column.

[fbe555]

See related issue: #554

[christophebedard]

Seems reasonable, but we cannot merge it right away if it affects uncrustify tests in the ROS 2 core, unless someone fixes them.

Let me run CI with this PR to see what fails, if any.

[christophebedard]

Pulls: #555
Gist: https://gist.githubusercontent.com/christophebedard/6f94f5b4bc54f2a4be1464388104b2e0/raw/4933bfa6f3e25db8dea9769478e44bd8ac37efee/ros2.repos
BUILD args: --packages-above-and-dependencies ament_uncrustify
TEST args: --packages-above ament_uncrustify
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/16697

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[christophebedard]

Seems reasonable, but we cannot merge it right away if it affects uncrustify tests in the ROS 2 core, unless someone fixes them.

if you only care about using this with your own package, you can simply point ament[_cmake]_uncrustify to this modified version of the config file:

• ament_uncrustify: use -c

  ament_lint/ament_uncrustify/ament_uncrustify/main.py

  Line 77 in 6353c1b

  '-c',

• ament_cmake_uncrustify: use CONFIG_FILE

  ament_lint/ament_cmake_uncrustify/cmake/ament_uncrustify.cmake

  Line 25 in 6353c1b

  # :param CONFIG_FILE: the path of the configuration file for

[fbe555]

I see your point. I already fixed it for my project, but I thought I would start the discussion. I don't know how prevalent nested classes are in the rclcpp lib, but considering all other packages as well, this might be disruptive.
If it's of interest, I'll gladly help out with refactoring. It shouldn't take long for any given repo, but then again, there are thousands and presumably a lot of them include the standard ament linters..

[christophebedard]

If we do want to merge this PR to be merged, all repos listed here have to be updated: https://github.com/ros2/ros2/blob/rolling/ros2.repos. Other (non-ROS core) repos can be updated, but that's not strictly required.

That being said, we indeed also have to consider that this is kind of a breaking change for downstream users. Is there a way to have uncrustify accept both ways, i.e., never indented or indented based on the indentation level of the corresponding class?

[fbe555]

I don't think you can make uncrustify have it both ways from what I read looking into the options today, but I'll try to find some time to look into it tomorrow.
I will also try and do some regex searching in the core repos to get a feel for how prevalent nested classes are. I suspect not very.
I'll comment here when I know more.

[fbe555]

Would it be possible to run ros core CI with the proposed config without merging, to see which repos break?

[christophebedard]

Would it be possible to run ros core CI with the proposed config without merging, to see which repos break?

That's what I did in this comment above: #555 (comment)

We're having issues with our CI so the test result/report page is disabled, but looking at the list of failed tests on the main job page, it looks like the following packages have uncrustify errors with this PR's uncrustify config:

• class_loader
• message_filters
• rclcpp
• rosbag2_cpp
• rosbag2_storage_sqlite3
• rosbag2_transport
• rqt_gui_cpp
• rviz_common
• rviz_default_plugins
• tf2_ros

If you click on "Console Output" on the left and then click "Full Log" or "View as plain text" you can find the test output to see what uncrustify is complaining about specifically. For example, here's one of the files uncrustify is complaining about for rclcpp:

5: Code style divergence in file 'include/rclcpp/experimental/timers_manager.hpp':
5: 
5: --- include/rclcpp/experimental/timers_manager.hpp
5: +++ include/rclcpp/experimental/timers_manager.hpp.uncrustify
5: @@ -206 +206 @@
5: -public:
5: +  public:
5: @@ -341 +341 @@
5: -private:
5: +  private:
5: @@ -353 +353 @@
5: -public:
5: +  public:
5: @@ -494 +494 @@
5: -private:
5: +  private:
5: 

[fbe555]

Sorry, I was on my phone yesterday, and missed the first comment. I still can't see a way to get uncrustify to accept both indentations, and I don't think there is one.
I will have a look at the failing repos tonight, to gauge the scale of the refactor.
This still leaves the question of downstream packages, and whether or not it is acceptable / worth it to make the change, breaking their CI.

[wjwwood]

From issue triage meeting, putting this on the ROS PMC meeting agenda due to it's potential for disruption to downstream users.

[christophebedard]

Worth noting, while it's still fresh: @wjwwood mentioned during the triage meeting that he sees this as more of a bug fix, because we should be properly indenting access specifiers. This could be used to justify breaking users' test, although this is so minor that maybe it's not worth breaking linter tests.

[christophebedard]

We discussed this in today's ROS PMC meeting.

In general, we agreed to move forward with this in Rolling. However, we should announce this change with a Discourse post. Probably in the ROS General category? The post should:

• Explain what the change is, why we want to do it (it's more of a bug fix), and its potential impact (ament_uncrustify test failures if default config and if indented classes)
• Include an ament_uncrustify ... --reformat example command, which should make migration very simple: https://github.com/ament/ament_lint/blob/rolling/ament_uncrustify/doc/index.rst#how-to-run-the-check-from-the-command-line
  • We should make sure to test this first

@fbe555 do you want to make the post? Otherwise, no worries, I can do it!

If the feedback we get is that users really don't want this disruption, then we can look into having ament_uncrustify try out more than 1 config file and pass if at least one of them is satisfied. This would allow us to easily migrate/change the config over time, but in practice that's probably not going to happen, so it's probably not worth implementing it if users are generally fine with this proposed change.

[fbe555]

@christophebedard I think it's best if you make the post. My response times will likely be a bit patchy for the next few weeks.

[peci1]

Will it be possible with this PR to develop multi-distro code in a single branch? Many projects want to have only a single branch for all supported ROS2 distros. Wouldn't this change make it impossible?

Or is there e.g. something like # noqa for uncrustify that would exclude the line from checking?

[fbe555]

There is an enable/disable comment that could be used to fence problematic sections. I haven't tested this, but it should work for this.

[christophebedard]

Yep! Great point, that use-case is important. Uncrustify ignores indentation for lines in between /* *INDENT-OFF* */ and /* *INDENT-ON* */. I've just tested it (and --reformat).

Or you can migrate to the new uncrustify config and use it (explicitly) with all distros. We could backport/copy this change to a new (non-default) uncrustify config file for older distros. However, it's probably simpler to use the above comments (vs explicitly setting the config file).

[peci1]

We could backport/copy this change to a new (non-default) uncrustify config file for older distros.

This would be great for downstream developers (ideally with a short tutorial on how to enable this config :) ).

The question is whether this copied file should not exist even in rolling to make this use-case as easy as possible. But in rolling it could be just a symlink.