-
-
Notifications
You must be signed in to change notification settings - Fork 6
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
document PR review feature caveats (#70)
This is mostly taken from a comment I posted in cpp-linter/cpp-linter-action#182 with some updates. Also reviewed other changes about the generated cli_args.rst document.
- Loading branch information
Showing
4 changed files
with
102 additions
and
28 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
Pull Request Review Caveats | ||
=========================== | ||
|
||
.. _repository settings: https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository#preventing-github-actions-from-creating-or-approving-pull-requests | ||
.. _organization settings: https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository#preventing-github-actions-from-creating-or-approving-pull-requests | ||
.. _hiding a comment: https://docs.github.com/en/communities/moderating-comments-and-conversations/managing-disruptive-comments#hiding-a-comment | ||
.. _resolve a conversion: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/commenting-on-a-pull-request#resolving-conversations | ||
|
||
.. hint:: | ||
|
||
This information is specific to GitHub Pull Requests (often abbreviated as "PR"). | ||
|
||
While the Pull Request review feature has been thoroughly tested, there are still some caveats to | ||
beware of when using Pull Request reviews. | ||
|
||
1. The "GitHub Actions" bot may need to be allowed to approve Pull Requests. | ||
By default, the bot cannot approve Pull Request changes, only request more changes. | ||
This will show as a warning in the workflow logs if the given token (set to the | ||
environment variable ``GITHUB_TOKEN``) isn't configured with the proper permissions. | ||
|
||
.. seealso:: | ||
|
||
Refer to the GitHub documentation for `repository settings`_ or `organization settings`_ | ||
about adjusting the required permissions for GitHub Actions's ``secrets.GITHUB_TOKEN``. | ||
2. The feature is auto-disabled for | ||
|
||
- closed Pull Requests | ||
- Pull Requests marked as "draft" | ||
- push events | ||
3. Clang-tidy and clang-format suggestions are shown in 1 Pull Request review. | ||
|
||
- Users are encouraged to choose either :std:option:`--tidy-review` or :std:option:`--format-review`. | ||
Enabling both will likely show duplicate or similar suggestions. | ||
Remember, clang-tidy can be configured to use the same ``style`` that clang-format accepts. | ||
There is no current implementation to combine suggestions from both tools (clang-tidy kind of | ||
does that anyway). | ||
- Each generated review is specific to the commit that triggered the Continuous Integration | ||
workflow. | ||
- Outdated reviews are dismissed but not marked as resolved. | ||
Also, the outdated review's summary comment is not automatically hidden. | ||
To reduce the Pull Request's thread noise, users interaction is required. | ||
|
||
.. seealso:: | ||
|
||
Refer to GitHub's documentation about `hiding a comment`_. | ||
Hiding a Pull Request review's summary comment will not resolve the suggestions in the diff. | ||
Please also refer to `resolve a conversion`_ to collapse outdated or duplicate suggestions | ||
in the diff. | ||
|
||
GitHub REST API does not provide a way to hide comments or mark review suggestions as resolved. | ||
|
||
.. tip:: | ||
|
||
We do support an environment variable named ``CPP_LINTER_PR_REVIEW_SUMMARY_ONLY``. | ||
If the variable is set to ``true``, then the review only contains a summary comment | ||
with no suggestions posted in the diff. | ||
4. If any suggestions did not fit within the Pull Request diff, then the review's summary comment will | ||
indicate how many suggestions were left out. | ||
The full patch of suggestions is always included as a collapsed code block in the review summary | ||
comment. This isn't a problem we can fix. | ||
GitHub won't allow review comments/suggestions to target lines that are not shown in the Pull | ||
Request diff (the summation of file differences in a Pull Request). | ||
|
||
- Users are encouraged to set :std:option:`--lines-changed-only` to ``true``. | ||
This will *help* us keep the suggestions limited to lines that are shown within the Pull | ||
Request diff. | ||
However, there are still some cases where clang-format or clang-tidy will apply fixes to lines | ||
that are not within the diff. | ||
This can't be avoided because the ``--line-filter`` passed to the clang-tidy (and ``--lines`` | ||
passed to clang-format) only applies to analysis, not fixes. | ||
- Not every diagnostic from clang-tidy can be automatically fixed. | ||
Some diagnostics require user interaction/decision to properly address. | ||
- Some fixes provided might depend on what compiler is used. | ||
We have made it so clang-tidy takes advantage of any fixes provided by the compiler. | ||
Compilation errors may still prevent clang-tidy from reporting all concerns. |