Using Statick to display required formatting changes

[jhdcs]

I'm not sure if this is the right place to put this sort of question, since I'm not sure if it's an issue with Staick per se, but here goes:

I am trying to get clang-format to work with Statick on an older version of Ubuntu (16.04) in a Docker container, but with a twist: due to work requirements, we don't want clang-format to change any of the files it's working on. This is solved from it being in Docker container, so all changes are discarded, but then I run into the issue that I can't see what changes were made. Currently, Statick just says something along the lines of "Made 1234 formatting changes", which doesn't give me a lot of information. I think that there's an option for clang-format to output changes to the standard output, but when I tried it outside of the container, it just seemed to drop the whole formatted file there. Plus, I'm not sure if that's captured by Statick...

The version of clang-format that I'm currently running (version 8) can output the changes in an XML file, but that's extremely difficult to parse, and I don't think it's documented anywhere how to parse it. Version 10 adds a "-dry-run" flag, but I'm having trouble getting anything beyond version 8 on the container. Seems to not see the repository.

So, I'm sort-of wondering what can be done. In addition to that, I'm not sure how this will affect the message reporting of Statick. Will it display the diffs correctly? There might be a situation where things are mis-aligned in the json file. Maybe.

I'm open to trying other formatters, but I think Uncrustify also just auto-changes the files too. Are there other formatters out there that would be useful to have included in Statick?

Again, I'm sorry if this is the wrong spot.

[tdenewiler]

This is a good spot to ask a question. I'll look into it, probably this afternoon.

[tdenewiler]

I'll explain how the plugin is designed and try to provide a path forward. If I miss anything you were asking about let me know.

The way that we originally designed the clang-format tool plugin was to get any changes the tool would make in XML format. Statick does not change any files in place. We then take that output and create a single issue per file, where the issue lists how many changes the tool would make in that file if it did in-place changes. We use line number 0 for the issue, indicating that the issue applies to the entire file, not any particular line. If we want to see the specific changes the clang-format tool would make then we run that tool outside of Statick. If we are happy with the changes then we run clang-format -i so that the changes are actually written to the file. After that Statick should not report any issues against that file.

The type of output I see from the print_to_console reporting plugin is

Tool clang-format: 2 unique issues
  /home/user/ws/src/pkg/src/x.cpp:0: clang-format:format: 3 replacements [1]
  /home/user/ws/src/pkg/src/y.cpp:0: clang-format:format: 12 replacements [1]

There are cases where a file is consistently formatted throughout, but given a new clang-format configuration there can be hundreds or thousands of lines of diff. This didn't seem useful to report beyond the fact that a single file had formatting differences. I just looked at the raw XML output and got the same feeling as you -- it would be difficult to parse.

The uncrustify tool is very similar to clang-format. I have not looked for other formatting tools for C++, but if you find one and want a plugin for Statick I would happy to look into writing a plugin.

Are you able to get the current output showing how many replacements would be made to a specific file? If that is not the information you are looking for in the output, what did you have in mind for more meaningful output? Does the approach of using Statick to get a pointer to the files that do not follow desired formatting work for you? This would be the approach where you use the clang-format tool outside of Statick to find the actual diffs.

[jhdcs]

Thank you for the detailed response!

Yes, that's the style of output I was seeing. I was hoping for an output similar to what you get in ROS / ROS 2 when using their linting code. Which, conveniently, I just found! https://github.com/ament/ament_lint/blob/master/ament_clang_format/ament_clang_format/main.py

For reference, what you see from that tool is something like this:

/path/to/file/example.cpp:277:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]

I am not sure how we could go about asking them permission to copy their work, if that's even what we want to do. Your point about there being hundreds or thousands of changes is a good one, I think. Perhaps if this is desired by other people, we could make it some sort of opt-in thing, maybe via a flag.

In the meantime, I could possibly work on whipping up something in-house...

[tdenewiler]

The amen_lint code uses the Apache2 license. I can copy their code into the Statick plugin as long as I leave in their copyright notice. I would also put in a link to their repository and give credit. If I do that, Statick could have a flag for the clang-format plugin that would output an issue per diff or a single issue per file. That flag would default to a single issue per file so that the default behavior of Statick does not change. If that sounds like a good path forward I would be happy to work on that in the next week or so.

[jhdcs]

That sounds excellent! I think they also do something similar for Uncrustify, just in case other users want similar functionality for that, too.

[tdenewiler]

@jhdcs, I was able to spend some time on this issue and will open a PR soon. When running Statick, If you use the flag --clang-format-issue-per-line the output will look similar to the following snippet (note that I artificially added these formatting issues to the example code). Is this the type of output that you had in mind?

INFO:root:---Reporting---
INFO:root:Running print_to_console reporting plugin...
Tool clang-format: 4 unique issues
  /home/thomas/src/node_example/src/node_example/src/talker.cpp:3: clang-format:format: Replace
- namespace node_example           
- {
with
+ namespace node_example
+ {
 [1]
  /home/thomas/src/node_example/src/node_example/src/talker.cpp:10: clang-format:format: Replace
-   cb = boost::bind(&ExampleTalker::configCallback, this, _1, _2);             
-   dr_srv_.setCallback(cb);
with
+   cb = boost::bind(&ExampleTalker::configCallback, this, _1, _2);
+   dr_srv_.setCallback(cb);
 [1]
  /home/thomas/src/node_example/src/node_example/src/talker.cpp:17: clang-format:format: Replace
-   // of the node can be run simultaneously while using different parameters.
-         ros::NodeHandle pnh("~");
with
+   // of the node can be run simultaneously while using different parameters.
+   ros::NodeHandle pnh("~");
 [1]
  /home/thomas/src/node_example/src/node_example/src/talker.cpp:40: clang-format:format: Replace
- void ExampleTalker::stop() {
with
+ void ExampleTalker::stop()
+ {
 [1]
4 total unique issues
INFO:root:print_to_console reporting plugin done.

The new per-line output will have more of a diff-style in the issue message that is reported.

The example output you showed before looks like it comes from ament_cpplint.

https://github.com/ament/ament_lint/blob/master/ament_cpplint/ament_cpplint/cpplint.py#L4314

[jhdcs]

Yes! I think that will work really well, provided it doesn't cause any problems with JSON outputs!

[tdenewiler]

I looked at the output of the print_json and write_jenkins_warnings_ng reporting plugins and it looks valid to me. I did not test the actual output in Jenkins (I don't have a test instance set up) but I don't see anything that would cause problems.

[tdenewiler]

These changes are now available in the latest release, v0.5.3.

https://pypi.org/project/statick/

Let me know if anything needs fixing.