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

update golangci-lint rules #252

Merged
merged 1 commit into from
Nov 22, 2023
Merged

update golangci-lint rules #252

merged 1 commit into from
Nov 22, 2023

Conversation

jmank88
Copy link
Collaborator

@jmank88 jmank88 commented Nov 22, 2023

This PR enables some new linters and revive rules for things we are already trying to do.

vyzaldysanchez
vyzaldysanchez previously approved these changes Nov 22, 2023
@jmank88
Copy link
Collaborator Author

jmank88 commented Nov 22, 2023

I was unable to configure testifylint to disable the require-error rule, which was producing many false positives, so I have removed testifylint for now.

@jmank88 jmank88 requested a review from nolag November 22, 2023 23:09
@jmank88 jmank88 marked this pull request as ready for review November 22, 2023 23:09
@patrickhuie19
Copy link
Contributor

Does this make #246 obsolete? I had a feeling something like this was coming so I didn't merge

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@ -39,7 +39,7 @@ func (a *atomicBroker) DialWithOptions(id uint32, opts ...grpc.DialOption) (conn
return a.load().DialWithOptions(id, opts...)
}

func (a *atomicBroker) NextId() uint32 {
func (a *atomicBroker) NextId() uint32 { //nolint:revive
Copy link
Contributor

Choose a reason for hiding this comment

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

what does this do?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ignores the naming failure on Id instead of ID, since we are implementing an interface from another package.

}
s.log.Errorw("failed to fetch from source", "error", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels odd to me - the previous formatting was easier to read

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://go.dev/doc/effective_go#control-structures

In the Go libraries, you'll find that when an if statement doesn't flow into the next statement—that is, the body ends in break, continue, goto, or return—the unnecessary else is omitted.

@jmank88
Copy link
Collaborator Author

jmank88 commented Nov 22, 2023

Does this make #246 obsolete? I had a feeling something like this was coming so I didn't merge

No, this doesn't affect CI other than the version bump.

Copy link
Contributor

@patrickhuie19 patrickhuie19 left a comment

Choose a reason for hiding this comment

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

Does this make #246 obsolete? I had a feeling something like this was coming so I didn't merge

If it does, I think this PR is a good opportunity to update the sonar properties:

sonar.projectName=chainlink-common

@jmank88 jmank88 merged commit 6953c5a into main Nov 22, 2023
6 of 8 checks passed
@jmank88 jmank88 deleted the golangci-lint-update branch November 22, 2023 23:58
samsondav pushed a commit that referenced this pull request Jan 11, 2024
ettec pushed a commit that referenced this pull request Mar 28, 2024
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

3 participants