search

MRtrix3 / mrtrix3

MRtrix3 provides a set of tools to perform various advanced diffusion MRI analyses, including constrained spherical deconvolution (CSD), probabilistic tractography, track-density imaging, and apparent fibre density
http://www.mrtrix.org
Mozilla Public License 2.0
294 stars 182 forks source link

clang-tidy Action issues #2844

Open Lestropie opened 8 months ago

Lestropie commented 8 months ago

  1. Is it possible to have the clang-tidy Action automatically hide any prior clang-tidy bot comments as "Outdated"? PRs can look quite spammy otherwise.

  2. The Action seems to run into hiccups whenever a force push is performed. It seems to take the warning & suggested change for a given line range for the out-of-date code, and propose that change to the same line range of the up-to-date code, which may not correspond (or in some cases the fix to the recommendation may have been implemented in the force push). I personally tend to force-push a bit because I don't like stacking commits to incrementally address issues that should have been addressed as a cohesive changeset. There's arguments for and against. But it's annoying regardless.

daljit46 commented 8 months ago

To my knowledge, these would require modifying the clang-tidy-review Github Action we are using. It's open source, so if we can do this easily enough I'm happy to make a PR on their repo.

More easily, one thing we can do is to limit the maximum number of comments that the action outputs. Currently, it's set to 25 but perhaps setting it to 10 may be more desirable. Additionally, we could disable the clang-tidy checks on large PRs (>=50 changed files) like #2829. Generally, speaking these PRs are often the result of formatting or simply and repetitive changes, thus the author of the PR is not usually responsible for large part of the code being modified. The idea of using clang-tidy in PRs has always been more about helping authors to improve their code, so I think this would make sense.

Lestropie commented 8 months ago

Could perhaps submit an Issue to the action repo to see if they think an (optional) automatic hiding of prior bot comments would be of interest to them. It seems such an obvious thing to me after only a small amount of use, I highly doubt I'm the first one to think of it.

The issue with force pushes may be more appropriately considered a bug, assuming I'm correct. Maybe need to start there with a clear working example on a fork.

Your point about the fundamental purpose of the Action is perhaps more literally interpreted as that it should ignore any changes authored by @MRtrixBot. Not sure if there's any merit to that concept, just a literal reading.

I think I'm content to leave settings as they are RE number of change suggestions shown / staying active even for large PRs. Can always hide comment / just leave unaddressed. One prospect that comes to mind that would potentially expedite things in this regard would be for the Action to group suggested changes based on rule, show only the first of those suggested changes, and indicate in the comment text that there are multiple recommended changes based on that rule, pointing to the logfile if the developer wishes to address all such instances. I expect that'd be a bit of work on the Action side. Could always just submit as a feature request if there's merit to the idea.