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 assorted oddities found by golangci-lint #1040

Merged
merged 3 commits into from Aug 3, 2022
Merged

Conversation

xrstf
Copy link
Contributor

@xrstf xrstf commented May 2, 2022

This PR fixes a lot of the errors found by various linters in golangci-lint, mostly focussing on

  • errorlint, ensuring errors are not compared with == or type assertions anymore, but errors.Is() and errors.As() are used.
  • strings.Replace(_, _, _, -1) == strings.ReplaceAll(_, _, _)
  • string(buf.Bytes()) == buf.String()
  • if strings.HasSuffix(a, b) { trim suffix } == strings.TrimSuffix()
  • reqParam and warnings in the unit tests were entirely unused.
  • I made the unit tests robust against people like me who trim spaces when saving a file. Previously some of the variables in the registry_test.go required trailing spaces in the Go code.

@@ -40,10 +40,8 @@ type apiTest struct {
inRes interface{}

reqPath string
reqParam url.Values
Copy link
Member

@kakkoyun kakkoyun May 2, 2022

Choose a reason for hiding this comment

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

Why are we removing these? Are they not used? Maybe there's something needs to be fixed in these tests.

Copy link
Contributor Author

@xrstf xrstf May 2, 2022

Choose a reason for hiding this comment

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

They are entirely unused, just like warnings was. It seemed there were other mechanisms in place to construct the request params (using the do field in each testcase)

Copy link
Contributor Author

@xrstf xrstf May 2, 2022

Choose a reason for hiding this comment

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

But I was also surprised that this amount of dead code would be here, so maybe the linter and me misinterpreted something? :)

Copy link
Member

@kakkoyun kakkoyun May 2, 2022

Choose a reason for hiding this comment

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

We might have changed the comparison method or something similar without realizing their intended use. Let's make sure if we can actually use them. Otherwise, happy to accept clean-ups.

Copy link
Contributor Author

@xrstf xrstf May 3, 2022

Choose a reason for hiding this comment

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

So from what I can see, the apiTest structure was introduced via #165 (released in v0.8.0). However I could not find a single revision in this repo that actually used the reqParam field.

Copy link
Member

@kakkoyun kakkoyun May 3, 2022

Choose a reason for hiding this comment

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

That's an ancient PR 😅 Thanks for digging it up. Then I'm ok with the changes. If anything becomes broken, we can fix them on consecutive PRs.

@kakkoyun
Copy link
Member

kakkoyun commented May 3, 2022

@xrstf One last could you please let me know which linters have you used for these changes? It would be even better if you update the .golangci.yaml file with them so that we can preserve consistency for subsequent changes.

@xrstf
Copy link
Contributor Author

xrstf commented May 3, 2022

No problemo. I first only fixed the low hanging fruit, as I did not intend for the linters to be enabled permanently. But if you want, gladly, we can make them permanent. I pushed a commit to enable them and fix the remaining not-so-low-hanging fruit. :)

@xrstf
Copy link
Contributor Author

xrstf commented May 3, 2022

Not super sure about the Github action failing, I have the exact same golangci-lint version locally and it finishes without issues.

@kakkoyun
Copy link
Member

kakkoyun commented Jun 17, 2022

@xrstf Could you, please rebase this one?
We merged #1056. I wonder how this looks after that one merged.

@xrstf
Copy link
Contributor Author

xrstf commented Jun 17, 2022

@kakkoyun Can do :)

@kakkoyun
Copy link
Member

kakkoyun commented Aug 2, 2022

@xrstf Do you think you can check this out this week? We want to cut a release.

xrstf added 2 commits Aug 2, 2022
Signed-off-by: Christoph Mewes <christoph@kubermatic.com>
Signed-off-by: Christoph Mewes <christoph@kubermatic.com>
Signed-off-by: Christoph Mewes <christoph@kubermatic.com>
@xrstf
Copy link
Contributor Author

xrstf commented Aug 2, 2022

I can!

In the meaintime there was #1047, but I still assume that since the reqParams are unused, I can still remove them in this PR?

@xrstf
Copy link
Contributor Author

xrstf commented Aug 2, 2022

Are the test failures are flakes? Locally make test seems to work just fine.

@kakkoyun
Copy link
Member

kakkoyun commented Aug 3, 2022

Are the test failures are flakes? Locally make test seems to work just fine.

It seems like it :) Now, it is fixed.

@kakkoyun kakkoyun merged commit 618194d into prometheus:main Aug 3, 2022
6 checks passed
@xrstf xrstf deleted the linting branch Aug 3, 2022
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.

None yet

2 participants