Possible to surface the reason/comment for rule suppression? #216
Comments
The CLI version isn't? 😛 ConfigThe config for this should be pretty simple, I think: you could take the current syntax: [tool.repo-review]
ignore = ["PC110"]and use this instead: [tool.repo-review.ignore]
PC110 = ""Due to the way dict iteration works in Python, this is backward compatible with the current versions of repo-review. Empty strings would be un-annotated, which is the old behavior. PS: We need to remove this ignore from this repo. The check works now. :) One worry here is how to mark each check. I would expect that this: [tool.repo-review.ignore]
MY = "This package uses pyright for type-checking instead"maybe should produce one line about MyPy checks being ignored, but it matches multiple checks, each with their own URLs, etc. Also if several ignores match, I guess we should just clearly document the behavior, like last one matches (or first, etc). APIWe can make arbitrary changes to the API if we need to, we use We could just read and output the Web versionThe example provided here is exactly what sp-repo-review uses, so this is the right place to update it. We'll just copy it over after a release. CLI versionI think we could add this to the output just like the web version. Some care might be needed to make sure the JSON output isn't a breaking change. Would the "simple" way work? Here's AstroPy's current config: [tool.repo-review]
ignore = [
"MY", # ignore MyPy setting checks
"PC111", # ignore using `blacken-docs` in pre-commit
"PC140", # ignore using `mypy` in pre-commit
"PC180", # ignore using `prettier` in pre-commit
"PC901", # ignore using custom update message (we have many of the default ones in our history already)
"PP308", # ignore requiring `-ra` flag for pytest, astropy's test suite is too large for this to be useful
]Imagining something like this: [tool.repo-review.ignore]
MY = "ignore MyPy setting checks"
PC111 = "ignore using `blacken-docs` in pre-commit"
PC140 = "ignore using `mypy` in pre-commit"
PC180 = "ignore using `prettier` in pre-commit"
PC901 = "ignore using custom update message (we have many of the default ones in our history already)"
PP308 = "ignore requiring `-ra` flag for pytest, astropy's test suite is too large for this to be useful"The "simple" version would just put something like this on the beginning or end: Ignored checks: MY: ignore MyPy setting checks Is that enough? Or would having the URLs, expanded checks (with repeated reasons), original reasons, etc. be useful? |
Haha, well I guess it speaks to the coolness of the hosted version. All from my phone and thus not having used the CLI yet, over the course of an hour I went from zero knowledge, to discovery via your Mastodon post, to running it against my Your description sounds about right, thanks for the detailed consideration! I like the non-breaking way of allowing a TOML table in place of an array due to the way that Python iterates mappings and sequences. One thing to consider is that allowing TOML tables would make it easier to accidentally give checks empty TOML tables like so: Warning Maybe you already handle these invalid configs in your parsing. If not, you'll want to either disallow that or silently handle it (or handle and warn) and consider it equivalent to an empty string. And yeah, representing check values in the resulting report may be non-trivial. The "simple" version you mention is totally reasonable. However, maybe it gets unwieldy for lots of ignores. Below the "details" fold I have rendered an example report for
But if any of these rules require significant refactor, it's totally fine to have either the "simple" version, or one with "reasons" repeated on each rule they apply to, for instance, rather than the more structured report shown below. In any case, thanks for taking the time to write up your conception of it, and for detailing some of the internals involved! [tool.repo-review.ignore]
GH21 = "some good reason for ignoring all the GH210s rules"
MY = "ignore MyPy setting checks"
PC111 = "ignore using `blacken-docs` in pre-commit"
PC140 = "ignore using `mypy` in pre-commit"
PC180 = "ignore using `prettier` in pre-commit"
PC901 = "ignore using custom update message (we have many of the default ones in our history already)"
PP308 = "ignore requiring `-ra` flag for pytest, astropy's test suite is too large for this to be useful"DetailsGeneral
PyProject
GitHub Actions
Pre-commit
MyPy [ignored]
Ruff
... |
Easy, that's what we already do.
Pretty easy, it does require a change to the api, though the simplest way would probably to have a new keyword argument to
This is hard, as this would affect the ordering, wouldn't be clear if multiple ignores matched a check, etc. Probably not impossible.
Also hard, as "family" and check name are not actually linked. You could have several codes inside one family, or even split codes across different families. A first version could do the "simple" thing, with a more advanced version as a followup eventually.
That's supported now, any iterable can be used. validate-pyproject can be used to validate the config, and we'd update our schema to allow either a list or strings or a table of strings, currently it's a list of strings. |
|
Yeah, even the "simple" approach will be nice, scarequotes around it because even with the clear path to it I'm sure it'll take a bit of time. If you already had a zillion rules and complex include/exclude machinery in place to specify arbitrarily complex intersections, the more involved solution would probably be a closer hill. But the tight integration between rules and your documentation is pretty cool, it seems very docs-first in a good way. Anyways this is a quality-of-life request, and it's cool the idea is hammered out, but of course there's no pressure to prioritize this. It certainly seems a reasonably good first issue for someone to pick up. I'd offer to work it into a PR but somehow I've managed to fall behind on a couple past-due "PR promises", so I don't want to overcommit and then never get around to it 😅 Thanks for helping me work out the idea, it could be a good first issue if you get any takers, otherwise I'll check back in next month and see if it's still unworked! |
|
I've added this issue to the summit planning doc, maybe someone can work on it there. :) |
The hosted version of
repo-reviewis really cool!Idea
I don't know whether this feature request is most appropriate for
repo-reviewcore or just the web plug-in itself, but I thought it would be nice to render rules that have been suppressed/skipped with a reason/message given by the maintainers.For instance, if I use
pyrightfor type-checking and Renovate for dependency management, I might suppress the mypy and Dependabot rules. But maybe I provide a reason for it in the tool config, and that gets rendered in the hosted web app checklist as a grayed out checkbox, like..➖ (suppressed) mypy This package uses pyright for type-checking instead
Rationale
I imagine it would be difficult to add checks for every conceivable combination of tools and best practices, so an "escape-hatch" that allows for self-documenting config about why certain rules are suppressed could be a nice middle-ground.
Caveats
But I also understand how that complicates the configuration story, from a simple list of suppressed strings to some messy combination of strings and string-string (rule-rationale) mappings. And the alternative of picking up reasons by parsing the TOML in a comments-aware way is probably not fun, either.
Breaking changes
It would be possible to implement this without breaking existing configs, but would make configs more complex and maintaining two ways of specifying rules could get ugly.
Alternative/workaround
I haven't tried this, but presumably a repo config suppressing certain rules simply won't render those rules in the web hosted report, which could make a repo look all green just by only selecting the set of passing checks. But instead of individual rationale, a single config item like
messagecould give maintainers a place to detail their specific repo health decisions that would render at the top ofrepo-reviewreports, including in the hosted version.Thanks for this! It's great that you not only signal potential repo issues, but also document the way to solve them. Infrastructure is such a big part of friction in projects, and also the hardest to justify spending time on, so having concise guidance is helpful.
The text was updated successfully, but these errors were encountered: