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

Fix all go-staticcheck warnings #1670

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

braydonk
Copy link
Contributor

@braydonk braydonk commented Apr 10, 2024

Description

This PR fixes all present go-staticcheck warnings in the codebase.

Everyone who I've requested a PR from was the last editor of code that was addressed according to a git blame.

Related issue

How has this been tested?

Confgenerator unit tests.

Checklist:

  • Unit tests
    • Unit tests do not apply.
    • Unit tests have been added/modified and passed for this PR.
  • Integration tests
    • Integration tests do not apply.
    • Integration tests have been added/modified and passed for this PR.
  • Documentation
    • This PR introduces no user visible changes.
    • This PR introduces user visible changes and the corresponding documentation change has been made.
  • Minor version bump
    • This PR introduces no new features.
    • This PR introduces new features, and there is a separate PR to bump the minor version since the last release already.
    • This PR bumps the version.

This PR fixes all present go-staticcheck warnings in the codebase.
Copy link
Member

@quentinmit quentinmit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are there changes in the golden files? If the output is changing, that's not just a lint change.

A number of these changes appear to be creating inconsistencies between similar functions (e.g. it appears to have removed unused function parameters, so now there are some functions that take (a, b, c) and other functions that just take (a, c) based on the current implementation of that function. I don't think that's actually an improvement.

@braydonk
Copy link
Contributor Author

Why are there changes in the golden files?

Most of the goldens that are changing are removing punctuation from errors. The only valid output that changed is oracledb because there was trailing whitespace in the file that my editor trims automatically.

A number of these changes appear to be creating inconsistencies between similar functions

This is because of the unused parameters check in gopls. These functions weren't using the context that was passed in, and since they aren't bound by any interface I removed them from the argument list.

I'm fine with turning off the error formatting check to restore goldens to their original form. For the unused parameters, we could use _ for those, but it seems arbitrary if they aren't bound by satisfying some interface. But using _ for context arguments is probably a fine heuristic.

@braydonk braydonk force-pushed the braydonk-fix-warnings branch 2 times, most recently from d0e9d90 to 0bfdfe7 Compare April 11, 2024 12:49
Staticcheck will now be configured to ignore error formatting in the
confgenerator package since we rely on that formatting for output.
Functions that used to take an named parameter context that was unused
has that argument restored as an unnamed parameter.
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