allow diff comments for PRs

[2bndy5]

I've been digging through GH's REST API, and it looks like we can do an auto-review for PRs. I have to keep digging for more details, but I'd like to switch from posting a PR comment to creating a multitude of review comments (PR-synchronize events will have to discard stale reviews from prior workflow runs). This can also be done for pushed commits 😉

Getting line number, file paths, and suggestions from clang-tidy & clang-format can be done, but it would take some extra dependencies. Clang-format can output suggested fixes in XML (only to stdout), and clang-tidy can output fix suggestions to a specified YAML file. Again, I need to investigate the best course of parsing these outputs from bash script.

[2bndy5]

I'm starting to think that parsing the diff for files can actually be done with bash script (using a combination of grep, tr, & sep commands).

Latest dilemma is parsing a files diff that's split into "hunks". For example, this commits diff

looks like this in the JSON payload

[{
  "files": [{
    "patch": "@@ -32,7 +32,7 @@ args=(\"$@\")\n FMT_STYLE=${args[0]}\n IFS=',' read -r -a FILE_EXT_LIST <<< \"${args[1]}\"\n TIDY_CHECKS=\"${args[2]}\"\n-# CLANG_VERSION=\"${args[3]}\"\n+CLANG_VERSION=\"${args[3]}\"\n \n echo \"GH_EVENT_PATH = $GITHUB_EVENT_PATH\"\n echo \"processing $GITHUB_EVENT_NAME event\"\n@@ -128,8 +128,8 @@ do\n       CLANG_CONFIG=\"--config\"\n    fi\n \n-   clang-tidy \"$filename\" \"$CLANG_CONFIG\" >> clang_tidy_report.txt\n-   clang-format -style=\"$FMT_STYLE\" --dry-run \"$filename\" >> clang_format_report.txt\n+   clang-tidy-\"$CLANG_VERSION\" \"$filename\" \"$CLANG_CONFIG\" >> clang_tidy_report.txt\n+   clang-format-\"$CLANG_VERSION\" -style=\"$FMT_STYLE\" --dry-run \"$filename\" >> clang_format_report.txt\n \n    echo \"Current Working Directory = $CWD\"\n    if [[ $(wc -l < clang_tidy_report.txt) -gt 0 ]]"
  }]
}]

---

What needs to happen is tell the clang-tools to only focus on the lines marked with a "+" sign. Then align the clang-tidy output with the lines that start with

@@ -<start line>,<number of lines> +<start line>,<number of lines> @@

Here is the link to the REST API docs that describe how to post a PR review comment(s). The REST API for creating a comment for commits (directly in the file or not) isn't much different.

[2bndy5]

I've written some python scripts that will parse

1. the XML output from clang-format (uses python's builtin XML parser module)
2. the YML output from clang-tidy. This does not use the 3^rd party PyYaml module as a dependency. However, I might have to switch to that dependency if I find that there are differences between YML outputs from different version of clang-tidy...

My intention is to keep using the run_checks.sh script unless the a new user input called "diff-only" is asserted (set to 'true'). Unlike other actions that rely on python to parse clang-* outputs, I'm trying to avoid adding more dependencies and putting all the algorithms into a long and confusing singular python script.

Should we switch to python completely?

I'm currently debating if we should completely switch over to python. This would replace the current dependencies jq & curl with python's 3^rd party requests module (python has inbuilt json module that we can use to replace jq).

ps - I have committed these scripts to my fork's parse-better branch, but I'm obviously still brainstorming my approach.

[2bndy5]

It looks like PyYAML is already installed in the ubuntu:latest image that the clang-tools container is built on. I should switch over...

[shenxianpeng]

I think your idea is very good. I haven't seen any other actions that have this feature before. It would be a cool thing to have it.

IMHO, we should keep the current way of comments to PR, comment in-file is an option for users. The reason is: If there is only comment in-file when users modify multiple files, there will be many comments on the code. they are mixed with review comments, which may be inconvenient to view. It will also cause a large number of notifications to be received. Looking forward to seeing the actual test results.

The core function of cpp-linter-action is to tell users that your code is not formatted or has programming errors. For format problems, if we can go further to help users automatically format (enabled by default), create a new formatted code, commit it to a new branch or create a new Pull Request directly, and associate it with the current branch or PR, this will help the developer to focus on coding.

A real experience from my project: different users’ development environments have different IDEs. engineers need to installed clang-tidy and clang-format in their environment locally, it hard for Windows and other Unix(like AIX, Solaris, HP-UX, etc) users or even the same version, and also need them to execute clang-format and clang-tidy automatically, which is very difficult. unless they set up the IDE correctly or have git hooks.

[shenxianpeng]

Should we switch to python completely?

I guess the answer is yes. shell does not seem to be satisfied when we want to support more features.

[2bndy5]

Done on the parse better branch. Coincidentally, this is the second time I made a python pkg installable for a github action... It's pretty complex under the hood, but its usage is very simple.

1. Install the python pkg copied over to docker image
2. Run the installed executable script as the docker entrypoint.

So far it works the same, but I think there's something wrong with posting the comments right now - never used the requests module to POST before. Building the action has slowed down by 10 seconds (minimum) though.

[shenxianpeng]

I used requests done much API-related work... Just see your branch, I marvel at how fast you write code.

[2bndy5]

Been a while since I made progress on this issue, so here is an update:

Parsing diffs is proving more difficult than expected because of different scenarios presented in diffs and how the REST API allocates comments in a diff.

I found out that github allows anyone to suggest changes in a PR's "Files Changed" tab.
A comment like this

this should be changed back when ready
```suggestion
      - uses: shenxianpeng/cpp-linter-action@master
```

will render like this

I'm not sure if BitBucket offers the same option (or similar). Notice the "commit suggestion" and "add suggestion to batch" buttons at the bottom of the rendered comment. These buttons allow the repo's maintainer(s) to directly commit changes from the comment(s) contents! 🥇

There is 1 drawback though: The suggestion is limited to only the lines that are displayed in the diff. Some suggestions from clang-tidy involve multiple lines (like adding { and } or replacing #pramga once with proper header guards and a closing #endif). This makes outputting multi-line suggestions harder to stay scoped within the diffs' hunks. For simplicity with multi-line suggestions, I'll probably just output the clang-tidy notification (kinda like what is output currently) within the diff instead of trying to suggest a multi-line fix.

[shenxianpeng]

Thank you for the update. It looks complex, and it's OK if it can implement or not.
I just tested Bitbucket, it also offers the same option. amazing!

[2bndy5]

I've decided to only do diff comments for PRs because any diff comments made to a commit's diff will show in a PR as review comments. I'm starting to think twice about supporting push events for this reason (a "synchronized" event for a PR behaves identically to push events for a commit - except its for a different thread's URL)... We'll just have to see how it looks.

[2bndy5]

just a teaser

[2bndy5]

I just found out that installing clang-tools comes with some handy extras like /bin/clang-tidy-diff.py (which outputs clang-tidy warnings for a given unified diff) and /bin/clang-format-diff.py (which can format changes in a given unified diff).I'm going to keep investigating, but using these extra tools requires a change to the docker image (installing clang-tools-<version> instead of clang-format-<version> and clang-tidy-<version> individually).

Status

I have clang-tidy fixes and clang-format fixes aggregated and output as diff comments (with suggestions where available). However, I've found that doing this manually yields incomplete/imprecise suggestions in some situations.

example:

The main problem is that the clang-tools outputs suggestions in terms of a file's offset (in bytes) instead of in terms of a file's line numbers.

Conclusion

I'm going to roll back changes that include parsing clang-tools suggestions. I created a new diff-only input option (defaults to false - meaning action's original behavior) runs clang-tools against only what's in the event's diff - not the entire file(s) mentioned in the event's diff. I can keep the new diff-only user input option, but I'm going to roll back any changes related to posting comments within the event's diff. Maybe we can re-visit this issue later...

[2bndy5]

I found out how code inspector is making annotations to each commit: it's using the github checks API (a subset of the REST API).

I've started experimenting with this checks API on a different repo, but it looks like there's a required app-specific secret token involved... Additionally, it looks like we can post a max of 50 annotations per payload (via requests module), and we can do subsequent https requests if more than 50 annotations are needed. This will help avoid the REST API's usage limit (max of 25 comments).