Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: enable unparam linter #558

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

G-Rath
Copy link
Collaborator

@G-Rath G-Rath commented Mar 13, 2025

This linter checks for unused functions and parameters, and parameters that have only a single value which is why I've introduced a private global variable in the containerd test; that feels a little weird, but I think it's slightly better than having a linter disable comment given it's internal test code.

I've already addressed the main two classes of violations this linter caught in the codebase, so the actual net win for enabling this is the sum of this + these PRs:

There was one new violation that slipped through after #520 was landed, which I've also addressed here

Relates to #274

@G-Rath G-Rath mentioned this pull request Mar 13, 2025
46 tasks
@G-Rath G-Rath force-pushed the linting/enable-unparam branch from f9a5ffb to 6bc973d Compare March 14, 2025 01:21
@another-rex
Copy link
Collaborator

Hmm the containerd one does seem pretty odd, I'm leaning towards adding a nolint there. Reasoning is that it looks like it is a helper function that can support adding additional different test files, just that right now there is only one.

It is much easier to remove the nolint later than it is to add the arguments back. (And we'll have the nolintlint to catch it).

I haven't looked into this too much though, pinging @andreyka who wrote this initially, is my assessment correct?

@G-Rath
Copy link
Collaborator Author

G-Rath commented Mar 14, 2025

Ironically that was originally using a temp dir, but it was hardcoded apparently to fix some issue with running on vms...

I agree it's odd, though I was concerned that nolinting it would disable the lint for the whole function signature rather than just that param

@G-Rath G-Rath force-pushed the linting/enable-unparam branch from 6bc973d to 40a90f6 Compare March 14, 2025 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants