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

Bump golangci-lint #610

Merged
merged 1 commit into from
Sep 11, 2021
Merged

Bump golangci-lint #610

merged 1 commit into from
Sep 11, 2021

Conversation

kuritka
Copy link
Collaborator

@kuritka kuritka commented Sep 8, 2021

closes #609
Golangci-lint has been greatly expanded with new linter types, some of which I have integrated.

With the new linters came also changes in the code, mainly

  • introduction of constants for repeating text
  • throwing out deprecated code

Some of the linters were not suitable and have been commented out

  • golint is deprecated, revive is instead
  • whitespace - it's bad to have empty lines in the code, in my opinion it degrades clarity
  • funlen - watches the length of functions and their names
  • exhaustive - completing missing constants in the switch (not only default)
  • gomnd - force to have any number in constant. E.g. mark if len(x) > 2 ,
  • gofmt - is part of goimports

I had to change linter configuration for these:

  • gochecknoinits - it is bothered by wrong use of init function - init() funcs in generated code
  • dupl - duplicates in the code (this only applies to testing, where duplicates happen frequently. We don't mind it there)

In addition I updated build pipeline.

Signed-off-by: kuritka kuritka@gmail.com

@kuritka
Copy link
Collaborator Author

kuritka commented Sep 8, 2021

@here: will require upgrade golangci locally: https://golangci-lint.run/usage/install/#macos

@kuritka kuritka force-pushed the bump-golangci branch 5 times, most recently from a0324f9 to 114462a Compare September 9, 2021 07:54
closes #609
Golangci-lint has been greatly expanded with new linter types, some of which I have integrated.

With the new linters came also changes in the code, mainly
 - introduction of constants for repeating text
 - throwing out deprecated code

Some of the linters were not suitable and have been commented out
 - golint is deprecated, revive is instead
 - whitespace - it's bad to have empty lines in the code, in my opinion it degrades clarity
 - funlen - watches the length of functions and their names
 - exhaustive - completing missing constants in the switch (not only default)
 - gomnd - force to have any number in constant. E.g. mark if len(x) > 2 ,
 - gofmt - is part of goimports

I had to change linter configuration for these:
 - gochecknoinits - it is bothered by wrong use of init function - it is only init() in generated code
 - dupl - duplicates in the code (this only applies to testing, where duplicates happen frequently. We don't mind it there)

In addition I updated build pipeline.

Signed-off-by: kuritka <kuritka@gmail.com>
Copy link
Member

@ytsarev ytsarev left a comment

Choose a reason for hiding this comment

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

Left couple of comments otherwise looks good


# don't enable:
# - golint # deprecated
# - whitespace
Copy link
Member

Choose a reason for hiding this comment

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

why not? :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had problem with empty lines we keep in the code, which delimits block of code, e.g.: we will remove empty lines here: this

@@ -33,7 +33,7 @@ import (

func TestFactoryInfoblox(t *testing.T) {
// arrange
client := fake.NewFakeClientWithScheme(scheme.Scheme, []runtime.Object{}...)
client := fake.NewClientBuilder().WithScheme(scheme.Scheme).WithRuntimeObjects([]runtime.Object{}...).Build()
Copy link
Member

Choose a reason for hiding this comment

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

Is it dictated by newer fake client version?

Copy link
Collaborator Author

@kuritka kuritka Sep 10, 2021

Choose a reason for hiding this comment

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

It's deprecated. see upstream function in controller runtime: uses NewClientBuilder anyway and comment says it's deprecated. one of the new linters found that.

Copy link
Contributor

@somaritane somaritane left a comment

Choose a reason for hiding this comment

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

lgtm

@kuritka kuritka merged commit 0c90866 into master Sep 11, 2021
@kuritka kuritka deleted the bump-golangci branch September 11, 2021 14:39
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.

Bump GOLANGCI-LINT
5 participants