Feedback about documentation

[atsju]

Is your idea related to an existing feature?

No response

Describe the behavior you would like

I'm a first time user of this wonderful action. It's really configurable and has overall good documentation.

One thing were I lost much time was the buttons on the documentation. Now that I know those are buttons, it's crystal clear BUT when I didn't know... it was impossible to find the information

This applies here :
https://cpp-linter.github.io/cpp-linter-action/permissions/#file-changes

and here
https://cpp-linter.github.io/cpp-linter-action/examples/

Also it would be great to have more recipes to see how to manage permissions:

• clang tidy problem matcher
• clang tidy individual comments
• clang tidy unique comment

Describe alternatives you have considered

• Change the shape of the documentation to make button more visible
• If not possible: don't use button and add more lines to documentation

Additional context

No response

[2bndy5]

One thing were I lost much time was the buttons on the documentation. Now that I know those are buttons, it's crystal clear BUT when I didn't know... it was impossible to find the information

The content tabs are styled by the mkdocs-material theme (which requires a separate mkdocs plugin to implement). We don't change their default appearance because, IMHO, they seem to be rendered in a rather standard way. I've seen other docs themes use the same styling. In the light scheme, I think the divider bar may be less visible than it is in dark scheme, but the selected tab indicator is very visible in either scheme.

Can link to an example of tabbed content in which the tabs are styled how you prefer/expect?

Also it would be great to have more recipes to see how to manage permissions

I don't really understand this. It seems like you expect the permissions to be different for each comment. But that is not how GitHub's token permissions are applied.

We documented how each feature of the action may change the expected permissions of the token. Beyond that, there's no additional granularity in permissions used for each individual comment posted.

I think what you are trying to say is: "What permissions are required for each cpp-linter-action feature?" For that answer, I would refer to the Inputs section of the docs. We added badges for inputs that require any special permission. For example:

Notice that these added badges (with the lock icon) are hyperlinked to the appropriate section is the "Token Permissions" page.

[2bndy5]

I tried playing around with the CSS for the content tabs... The indicator width and offset is set dynamically using JS and the buttons themselves are actually check-boxes (with the actual checkbox hidden leaving the text as the tab title). The hamburger navigation menu (typically seen on mobile or narrow browser windows) also uses this "hidden check-boxes" approach.

Ultimately, styling the tabs more like buttons would be too complex for a simple solution.
I'd rather just take the other suggested solution and separate the tabbed content into separate sections. However, this will add duplicate section headers to the page ToC (on the right side if viewing from desktop).

[atsju]

Hi,
Thank you for taking time to answer.

• about the tabs. Now I agree with you that it is clearly a tab. But now we are both biased by the fact we know it. When I looked at it first time I simply couldn't figure it out. It's only after making GPT read the doc and explain me that I understood how to find the information.
  I don't know anything about CSS and style. I suppose something like this would be clearer:

• About the permissions: I agree it's quite clear. My question was indeed more or less "What permissions are required for each cpp-linter-action feature?"
  In fact during my tests I have only been unable to understand the metadata: read. It gave me an error but test has been done on a pull request. So I don't quite understand what is the on: push case.
  You also say that you have badges explaining the permissions and I don't find the metadata badge so this adds to my misunderstanding.

[2bndy5]

1. Yikes! That is ugly as hell. The docs theme we use follows material style guidelines established by Google (see material tabs specification). You see these styles in most Android apps (some apps for iOS as well).
2. Some of your confusion comes from misunderstanding GitHub's CI triggers and token permissions.
   • A push event is specific to a branch. A pull_request event is specific to a PR. A push event can be triggered during a pull_request event (types: [synchronize]), but the context of the push event is very different from the context of a pull_request event. For example, cpp-linter-action does not have access to the PR comments during a push event, even if triggered simultaneously with a pull_request synchronize event type.
   • The metadata: read permission is always the bare minimum granted to every token.
   • A caveat inherent to all secrets.GITHUB_TOKENs is that comments for PR cannot be posted if the PR is coming from a fork. The is a limitation mandated by GitHub for security reasons.

[atsju]

It's a bit clearer thank you again.

One additional question. I'm testing format-review and so far I haven't achieved the "patch" PR comment
https://raw.githubusercontent.com/cpp-linter/cpp-linter-action/main/docs/images/format-review.png

To be clear, should I expect the action to generate a git patch file that I could then apply at end of PR process ?
Failing the PR as long as user hasn't executed clang-format is fine but I there could be a path available to apply on demand it's even better.
I will continue testing/learning, I have read in your documentation that draft PR might hinder some things but I have seen no problem so far.

[2bndy5]

The given patch for a PR review summary is meant to highlight that some suggestions could not be posted within the PR's diff. See our "Pull Request Review Caveats" doc for more rationale.

To be clear, should I expect the action to generate a git patch file that I could then apply at end of PR process ?

Only for our PR review features (tidy-review and format-review), not for any of our other features.

The patch should be applied before merging the PR to avoid repeated suggestions in the future (possibly in another PR too).

but I there could be a path available to apply on demand it's even better.

Typos aside, this feature request is similar to an idea that we've had about auto-applying clang tool changes. See cpp-linter/cpp-linter#82

I have read in your documentation that draft PR might hinder some things but I have seen no problem so far.

Yes, cpp-linter-action will not post PR reviews for PRs that are marked as drafts (aka "Not ready for review"). The thread-comments feature should still work fine in a draft PR (assuming the PR is not coming from a fork).

[atsju]

If I recap:

• file-annotations: annotation like for compile warning with problem matcher
• thread-comments: single comment in PR. For clang-format it lists failing files, for clang-tidy it lists warnings
• step-summary: I haven't tested but probably similar to thread-comments
• tidy-review: comments/suggestions about tidy + patch only if suggestion doesn't fit diff
• format-review: comments/suggestions about format + patch only if suggestion doesn't fit diff

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).

That's partially outdated. Github now permits to comment on lines not shown in the PR diff. However it's not the case in GitHub Enterprise so far... So don't be too fast to change behavior.

---

My dream was to get tidy-review comments + a patch for clang-format without comments. However, if we suppose the code is already formatted, the suggestion makes sense. Well I think the whole cpp-linter/cpp-linter#82 makes sense. How to be able to manage tidy and format at once.

---

maybe it makes sense to have a downloadable patch ? just thinking, not sure somebody actually apply this patches

[2bndy5]

step-summary: I haven't tested but probably similar to thread-comments

I typically recommend step-summary in situations where thread-comments is not allowed (because of PR from a fork) because it is the same content as thread-comments without requiring special permissions. Instead of seeing a posted comment, step-summary is part of the CI run summary page (just above where the CI artifacts can be downloaded).

Github now permits to comment on lines not shown in the PR diff

I think the wording could be improved. GitHub will not allow suggestions to be posted outside the diff. Yes, comments can be posted outside the diff, but not comments with suggestions.

maybe it makes sense to have a downloadable patch ? just thinking, not sure somebody actually apply this patches

This idea is getting off topic. I'm not sure how useful this would be. We haven't had any people give feedback on the patches posted in a collapsed block.

On the other hand, uploading patch file(s) could be useful to keep the PR review summary comment's length within the limits of GitHub's maximum 65535 characters. Instead of posting the code block, just hyperlink to the uploaded artifact.