PR Review Suggestions

[2bndy5]

should resolve #50

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3cca935) 99.90% compared to head (15d87a7) 100.00%.

Additional details and impacted files

@@             Coverage Diff             @@
##             main       #51      +/-   ##
===========================================
+ Coverage   99.90%   100.00%   +0.09%     
===========================================
  Files          18        19       +1     
  Lines        1099      1311     +212     
===========================================
+ Hits         1098      1311     +213     
+ Misses          1         0       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[2bndy5]

Seems there's a problem with using pygit2 for this. It has something to do with how the C libgit2 creates a patch using reference pointers to the buffers it is comparing. Meaning after the run_clang_***() exits, the references (to the original buffers that were diff'd) no longer exist.

I was able to reproduce this problem by altering the changes here in run_clang_format():

    if format_review:
        del cmds[2]  # remove `--output-replacements-xml` flag
        # get formatted file from stdout
        formatted_output = subprocess.run(cmds, capture_output=True)
        print("OLD:", Path(file_obj.name).read_bytes())
        print("NEW:", formatted_output.stdout)
        # create and store a patch between original file and formatted_output
        advice.suggestion = Patch.create_from(
            old=Path(file_obj.name).read_bytes(),
            new=formatted_output.stdout,
            old_as_path=file_obj.name,
            new_as_path=file_obj.name,
            context_lines=0,  # trim as many unchanged lines from diff as possible
        )
        print("PATCH:", advice.suggestion.data)
    return advice

Then when I run the code:

>>> from cpp_linter.common_fs import FileObj
>>> from shutil import which
>>> from cpp_linter.clang_tools.clang_format import run_clang_format
>>> f = FileObj("tests/demo/demo.cpp")
>>> cmd = which("clang-format")
>>> advice = run_clang_format(cmd, f, style="llvm", lines_changed_only=0, format_review=True)
OLD: b'/** This is a very ugly test code (doomed to fail linting) */\n#include "demo.hpp"\n#include <stdio.h>\n\n\n\n\nint main(){\n\n    for (;;) break;\n\n\n    printf("Hello world!\\n");\n\n\n\n\n    return 0;}\n'
NEW: b'/** This is a very ugly test code (doomed to fail linting) */\n#include "demo.hpp"\n#include <stdio.h>\n\nint main() {\n\n  for (;;)\n    break;\n\n  printf("Hello world!\\n");\n\n  return 0;\n}\n'
PATCH: b'diff --git a/tests/demo/demo.cpp b/tests/demo/demo.cpp\nindex 1bf553e..dca5ecb 100644\n--- a/tests/demo/demo.cpp\n+++ b/tests/demo/demo.cpp\n@@ -4,0 +5 @@\n+int main() {\n@@ -5,0 +7,2 @@\n+  for (;;)\n+    break;\n@@ -6,0 +10 @@\n+  printf("Hello world!\\n");\n@@ -8,11 +12,2 @@\n-int main(){\n-\n-    for (;;) break;\n-\n-\n-    printf("Hello world!\\n");\n-\n-\n-\n-\n-    return 0;}\n+  return 0;\n+}\n'
>>> # now print the patch data again after exiting run_clang_format()
>>> a.suggestion.data
b'diff --git a/tests/demo/demo.cpp b/tests/demo/demo.cpp\nindex 1bf553e..dca5ecb 100644\n--- a/tests/demo/demo.cpp\n+++ b/tests/demo/demo.cpp\n@@ -4,0 +5 @@\n+int main() {\n@@ -5,0 +7,2 @@\n+  for (;;)\n+    break;\n@@ -6,0 +10 @@\n+  printf("Hello world!\\n");\n@@ -8,11 +12,2 @@\n-\xf8\xda\xf0\x11\x02\x00\x00\x00\x00\x00\x00\x00-\x00-\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00-\x00-\x00-\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x97\x00e\x00j\x01\x00\x00\x00\x00\x00\x00\x00\x00j\x02\x00\x00-\x00-\x00-\x00-\x00-\x00\x00F\x00d\x00S\x00\x00\x00 0;}\n+  return 0;\n+}\n'

As you can see the un-formatted buffer, formatted buffer, and resulting patch buffer are all correct while within the scope of the function. But when the un-formatted and formatted buffers are garbage collected the resulting patch buffer gets corrupted when used after exiting run_clang-format.

I'm looking into a workaround now... But at least the REST API calls work to post a PR review (so far).

[2bndy5]

Well, this is starting to behave as expected. Apparently, dismissing an old review does not "resolve" (or auto-hide) the comments it contains.

1. I'm going to have to rethink how to update existing reviews (that were generated by our code).
2. I also have to add distinction about where the posted suggestion is coming from: "Is clang-tidy saying this? Or is clang-format saying this?"

But I finally got the line numbers to match up! I think it would be better if users only enabled reviews for runs that set lines-changed-only to true. Maybe I can hard-code that behavior into the review process.

[2bndy5]

I'm ready to merge this. I still want to do another round of QA testing from main branch. I haven't tested a PR where an APPROVED review is posted (yet). The unit test does cover that APPROVED condition (in a brute force way), but it would be good to see how that renders in an actual PR thread.

The new unit test can probably get improved... 96 separate tests from 1 parametrized test function seems a bit much.

$ pytest tests/reviews/test_pr_review.py

# ...

================================ 96 passed in 29.88s 

[2bndy5]

From this CI run in which I simulated an APPROVED condition:

response returned 422 from https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls/27/reviews with message: {"message":"Unprocessable Entity","errors":["GitHub Actions is not permitted to approve pull requests."],"documentation_url":"https://docs.github.com/rest/pulls/reviews#create-a-review-for-a-pull-request"}

So, apparently we can't approve a PR using secrets.GITHUB_TOKEN (with default permissions). It should be possible with a user's personal access token.

Note: The outdated reviews were still dismissed when the PR was APPROVED.

[2bndy5]

So, I tried to generate a token in my account settings on behalf of the cpp-linter org...

Then I fed the token as a secret into the test-repo's workflow, and I got this result:

returned 422 from https://api.github.com/repos/cpp-linter/test-cpp-linter-action/pulls/27/reviews with message: {"message":"Unprocessable Entity","errors":["Can not approve your own pull request"],"documentation_url":"https://docs.github.com/rest/pulls/reviews#create-a-review-for-a-pull-request"}

🤦🏼‍♂️ I forgot that I was the one that opened the PR. 🤣

[2bndy5]

This is what the APPROVED review comment would look like (extracted from test artifact):

---

Cpp-linter Review

No objections from clang-format.
No objections from clang-tidy.

Great job!

Have any feedback or feature suggestions? Share it here.

[2bndy5]

This github doc helped me fix the problem with token permission. Maybe it should get added to the cpp-linter-action repo's README when all this gets merged upstream.