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

enable gocritic linter #4412

Merged
merged 3 commits into from
Apr 27, 2023
Merged

enable gocritic linter #4412

merged 3 commits into from
Apr 27, 2023

Conversation

mmorel-35
Copy link
Contributor

Short description of the changes

  • enable gocritic linter
  • disabled dupArg, exitAfterDefer and appendAssign rules of gocritic until they are addressed.

Signed-off-by: Matthieu MOREL [email protected]

@mmorel-35 mmorel-35 requested a review from a team as a code owner April 25, 2023 10:50
@mmorel-35 mmorel-35 requested a review from joe-elliott April 25, 2023 10:50
@codecov
Copy link

codecov bot commented Apr 25, 2023

Codecov Report

Patch coverage: 97.56% and project coverage change: -0.02 ⚠️

Comparison is base (48e1438) 97.08% compared to head (1daba3a) 97.07%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4412      +/-   ##
==========================================
- Coverage   97.08%   97.07%   -0.02%     
==========================================
  Files         300      300              
  Lines       17730    17728       -2     
==========================================
- Hits        17214    17210       -4     
- Misses        415      417       +2     
  Partials      101      101              
Flag Coverage Δ
unittests 97.07% <97.56%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cmd/collector/app/zipkin/zipkindeser/json.go 97.25% <0.00%> (ø)
cmd/agent/app/testutils/in_memory_reporter.go 100.00% <100.00%> (ø)
cmd/env/command.go 100.00% <100.00%> (ø)
cmd/es-index-cleaner/app/index_filter.go 100.00% <100.00%> (ø)
cmd/ingester/app/flags.go 100.00% <100.00%> (ø)
cmd/query/app/query_parser.go 100.00% <100.00%> (ø)
model/span.go 100.00% <100.00%> (ø)
pkg/bearertoken/http.go 100.00% <100.00%> (ø)
pkg/config/tlscfg/flags.go 100.00% <100.00%> (ø)
pkg/metrics/metrics.go 100.00% <100.00%> (ø)
... and 10 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

albertteoh
albertteoh previously approved these changes Apr 25, 2023
Copy link
Contributor

@albertteoh albertteoh left a comment

Choose a reason for hiding this comment

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

Some nice improvements, thanks!

return fmt.Errorf(
"Field [%s]: Buckets should only be defined for Timer and Histogram metric types",
field.Name)
}
}
help := field.Tag.Get("help")
var obj interface{}
if field.Type.AssignableTo(counterPtrType) {
switch {
Copy link
Contributor

Choose a reason for hiding this comment

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

I tend to prefer the switch statement instead of if/else if chains because it's a bit cleaner and easier to read, but others maintainers may disagree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I started to code with Java I used to be one of those. But different language, different best practices and switch case is one of those differences. Gocritic is widely used which means that the golang community follow it’s rules. If the majority of the developers of this project don’t want to conform to this rule it can be excuded in the golangci-lint config file.
By the way, I’m not aware of any golang linter forbidding switch cases let me know if you do ;)

Copy link
Member

Choose a reason for hiding this comment

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

I rarely used switch in this manner, but why not. Readability is slightly higher, less boilerplate

@yurishkuro yurishkuro enabled auto-merge (squash) April 25, 2023 20:29
Signed-off-by: Matthieu MOREL <[email protected]>
Signed-off-by: Matthieu MOREL <[email protected]>
Signed-off-by: Matthieu MOREL <[email protected]>
auto-merge was automatically disabled April 27, 2023 06:45

Head branch was pushed to by a user without write access

@yurishkuro yurishkuro merged commit b4d2581 into jaegertracing:main Apr 27, 2023
@mmorel-35 mmorel-35 deleted the linters-gocritic branch April 27, 2023 20:18
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.

3 participants