document PR review feature caveats (#70)

2bndy5 · web-flow · commit 7dce923c0ed3 · 2024-02-12T00:45:36.000-08:00
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.
diff --git a/cpp_linter/cli.py b/cpp_linter/cli.py
@@ -14,7 +14,7 @@
     ),
     formatter_class=argparse.RawTextHelpFormatter,
 )
-arg = cli_arg_parser.add_argument(
+cli_arg_parser.add_argument(
     "-v",
     "--verbosity",
     type=lambda a: a.lower() in ["debug", "10"],
@@ -61,7 +61,10 @@
 - Set this to ``file`` to have clang-format use the
   closest relative .clang-format file.
 - Set this to a blank string (``""``) to disable
-  using clang-format entirely.""",
+  using clang-format entirely.
+
+See `clang-format docs <https://clang.llvm.org/docs/ClangFormat.html>`_ for more info.
+""",
 )
 cli_arg_parser.add_argument(
     "-c",
@@ -87,7 +90,7 @@
 
     %(default)s
 
-See also clang-tidy docs for more info.""",
+See also `clang-tidy docs <https://clang.llvm.org/extra/clang-tidy>`_ for more info.""",
 )
 arg = cli_arg_parser.add_argument(
     "-V",
@@ -105,7 +108,7 @@
 Default is """,
 )
 assert arg.help is not None
-arg.help += "a blank string." if not arg.default else f"``{arg.default}``."
+arg.help += f"``{repr(arg.default)}``."
 arg = cli_arg_parser.add_argument(
     "-e",
     "--extensions",
@@ -151,10 +154,10 @@
 - Glob patterns are not supported here. All asterisk
   characters (``*``) are literal.""",
 )
-arg = cli_arg_parser.add_argument(
+cli_arg_parser.add_argument(
     "-l",
     "--lines-changed-only",
-    default=0,
+    default="false",
     type=lambda a: 2 if a.lower() == "true" else int(a.lower() == "diff"),
     help="""This controls what part of the files are analyzed.
 The following values are accepted:
@@ -165,10 +168,8 @@
 - ``diff``: All lines in the diff are analyzed
   including unchanged lines but not subtractions.
 
-Defaults to """,
+Defaults to ``%(default)s``.""",
 )
-assert arg.help is not None
-arg.help += f"``{str(bool(arg.default)).lower()}``."
 cli_arg_parser.add_argument(
     "-f",
     "--files-changed-only",
@@ -282,21 +283,21 @@
 explicitly ignored domains (see :std:option:`--ignore`).""",
 )
 cli_arg_parser.add_argument(
+    "-d",
     "--tidy-review",
-    "-tr",
     default="false",
     type=lambda input: input.lower() == "true",
-    help="""Set to ``true`` to enable PR review suggestions
+    help="""Set to ``true`` to enable Pull Request reviews
 from clang-tidy.
 
 Defaults to ``%(default)s``.""",
 )
 cli_arg_parser.add_argument(
+    "-m",
     "--format-review",
-    "-fr",
     default="false",
     type=lambda input: input.lower() == "true",
-    help="""Set to ``true`` to enable PR review suggestions
+    help="""Set to ``true`` to enable Pull Request reviews
 from clang-format.
 
 Defaults to ``%(default)s``.""",
diff --git a/docs/conf.py b/docs/conf.py
@@ -6,7 +6,6 @@
 import sys
 import re
 from pathlib import Path
-import io
 from sphinx.application import Sphinx
 
 sys.path.insert(0, str(Path(__file__).parent.parent))
@@ -115,22 +114,26 @@
 def setup(app: Sphinx):
     """Generate a doc from the executable script's ``--help`` output."""
 
-    with io.StringIO() as help_out:
-        cli_arg_parser.print_help(help_out)
-        output = help_out.getvalue()
+    output = cli_arg_parser.format_help()
     first_line = re.search(r"^options:\s*\n", output, re.MULTILINE)
     if first_line is None:
         raise OSError("unrecognized output from `cpp-linter -h`")
     output = output[first_line.end(0) :]
     doc = "Command Line Interface Options\n==============================\n\n"
-    CLI_OPT_NAME = re.compile(r"^\s*(\-\w)\s?\{?[A-Za-z_,]*\}?,\s(\-\-.*?)\s")
+    doc += ".. note::\n\n    These options have a direct relationship with the\n    "
+    doc += "`cpp-linter-action user inputs "
+    doc += "<https://github.com/cpp-linter/cpp-linter-action#optional-inputs>`_. "
+    doc += "Although, some default values may differ.\n\n"
+    CLI_OPT_NAME = re.compile(
+        r"^\s*(\-[A-Za-z]+)\s?\{?[A-Za-z_,0-9]*\}?,\s(\-\-[^\s]*?)\s"
+    )
     for line in output.splitlines():
         match = CLI_OPT_NAME.search(line)
         if match is not None:
             # print(match.groups())
             doc += "\n.. std:option:: " + ", ".join(match.groups()) + "\n\n"
             options_match = re.search(
-                r"\-\w\s\{[a-zA-Z,]+\},\s\-\-[\w\-]+\s\{[a-zA-Z,]+\}", line
+                r"\-\w\s\{[a-zA-Z,0-9]+\},\s\-\-[\w\-]+\s\{[a-zA-Z,0-9]+\}", line
             )
             if options_match is not None:
                 new_txt = options_match.group()
diff --git a/docs/index.rst b/docs/index.rst
@@ -4,11 +4,7 @@
    :hidden:
 
    self
-   building_docs
-
-.. toctree::
-   :hidden:
-
+   pr_review_caveats
    cli_args
 
 .. toctree::
@@ -26,8 +22,7 @@
    API-Reference/cpp_linter.loggers
    API-Reference/cpp_linter.common_fs
 
-Indices and tables
-==================
+.. toctree::
+   :hidden:
 
-* :ref:`genindex`
-* :ref:`modindex`
+   building_docs
diff --git a/docs/pr_review_caveats.rst b/docs/pr_review_caveats.rst
@@ -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.
