Skip to content

MINOR: Make Checkstyle more strict, restore global code quality checks to 2018#15367

Closed
gharris1727 wants to merge 31 commits intoapache:trunkfrom
gharris1727:minor-cleanup-suppressions
Closed

MINOR: Make Checkstyle more strict, restore global code quality checks to 2018#15367
gharris1727 wants to merge 31 commits intoapache:trunkfrom
gharris1727:minor-cleanup-suppressions

Conversation

@gharris1727
Copy link
Copy Markdown
Contributor

@gharris1727 gharris1727 commented Feb 14, 2024

The connect suppressions are more permissive than necessary, because suppressions were added to the file and then not later removed. Most notably:

Recently PRs have been relaxing the global checkstyle limits instead of adding suppressions:

I noticed this lead to the DistributedHerder no longer needing a MethodLength suppression. While the heuristics are arbitrary, in the interest in preventing the "code quality ratchet" from slipping, I reverted those changes and added the necessary suppressions.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@gharris1727 gharris1727 changed the title MINOR: Remove Connect suppressions which are unnecessary MINOR: Make Checkstyle more strict for Connect, restore global code quality checks to 2018 Feb 14, 2024
@gharris1727 gharris1727 force-pushed the minor-cleanup-suppressions branch from 5b34f3b to d066773 Compare February 16, 2024 03:30
Signed-off-by: Greg Harris <greg.harris@aiven.io>
Signed-off-by: Greg Harris <greg.harris@aiven.io>
Signed-off-by: Greg Harris <greg.harris@aiven.io>
Signed-off-by: Greg Harris <greg.harris@aiven.io>
Signed-off-by: Greg Harris <greg.harris@aiven.io>
Signed-off-by: Greg Harris <greg.harris@aiven.io>
Signed-off-by: Greg Harris <greg.harris@aiven.io>
Signed-off-by: Greg Harris <greg.harris@aiven.io>
Signed-off-by: Greg Harris <greg.harris@aiven.io>
Signed-off-by: Greg Harris <greg.harris@aiven.io>
@gharris1727 gharris1727 force-pushed the minor-cleanup-suppressions branch from d066773 to 1e25705 Compare February 16, 2024 03:40
@gharris1727
Copy link
Copy Markdown
Contributor Author

I realized that it was a bit silly both manually auditing these, and only fixing the Connect ones. I've instead addressed all of the unused suppressions by building a little audit script.

It finds suppressions which:

  1. Don't use the full name of the check, and instead use a shortened form
  2. Include the same check and the same class as another suppression
  3. Are missing, and the class has errors (this is what checkstyle normally does)
  4. Are missing, the class has errors for, but a shorter class name is listed in the suppressions
  5. Are present, but the named class doesn't generate the corresponding error

It's a rough-and-ready script, and doesn't handle the more powerful regexes in the suppressions file, only the basic ones composed of a single group. There are carve outs for the powerful regex hardcoded-in that prevent them from emitting spurious errors.

I'll do my best to keep this rebased, but I would appreciate a quick review as people are currently adding new suppressions while this is open :)

@gharris1727 gharris1727 force-pushed the minor-cleanup-suppressions branch from 1e25705 to b1b6a5a Compare February 16, 2024 16:05
@gharris1727 gharris1727 changed the title MINOR: Make Checkstyle more strict for Connect, restore global code quality checks to 2018 MINOR: Make Checkstyle more strict, restore global code quality checks to 2018 Feb 16, 2024
@gharris1727 gharris1727 force-pushed the minor-cleanup-suppressions branch from b1b6a5a to e8787ab Compare February 16, 2024 16:24
@gharris1727
Copy link
Copy Markdown
Contributor Author

While I can sorta keep up with the stream of merge conflicts in this one PR, I realize now that completely rewriting nearly every line in suppressions.xml is going to generate a ton of merge conflicts for everyone else. This might to lead to build breakages, so I'm hesitating on merging this.

I'm if i should merge this as-is, close it un-merged, or scale it back to just the changes in checkstyle.xml (lowering the limits).

@github-actions
Copy link
Copy Markdown

This PR is being marked as stale since it has not had any activity in 90 days. If you would like to keep this PR alive, please ask a committer for review. If the PR has merge conflicts, please update it with the latest from trunk (or appropriate release branch)

If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 30 days, it will be automatically closed.

@github-actions github-actions Bot added the stale Stale PRs label Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale Stale PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants