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

golangci-lint not working correctly #2987

Closed

Conversation

Petrie
Copy link
Contributor

@Petrie Petrie commented Jun 29, 2022

There's two main changes

  • Update golangci-lint version to semver manner in go.mod . because dependabot is not able to update non-semver dependency.
  • Manual upgrade golangci-lint version to fix some linters not working correctly. Some linters not working correctly on version v1.45.3-0.20220330010013-d2ccc6d2bbbb, I don't why, but after upgrade, it works.

Additionally

As a freshman for metric, i'm not sure this changes is correctly for fix linter on sdk/metric/aggregator/lastvalue/lastvalue_test.go:79

Related link:

blizzy78/varnamelen#13 (comment)
#2761

@@ -76,7 +76,7 @@ func TestLastValueUpdate(t *testing.T) {

var last number.Number
for i := 0; i < count; i++ {
x := profile.Random(rand.Intn(1)*2 - 1)
x := profile.Random(rand.Intn(2)*2 - 1)
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't feel right. What's the lint error being raised here and why is this the approach taken to fix it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The lint error is

aggregator/lastvalue/lastvalue_test.go:79:24: SA4030: math/rand.Intn(n) generates a random value 0 <= x < n; that is, the generated values don't include n; rand.Intn(1) therefore always returns 0 (staticcheck)
			x := profile.Random(rand.Intn(1)*2 - 1)
			                    ^

As my first comment says , As a freshman for metric, I'm not sure this changes is correctly. pls make some advice directly.

@codecov
Copy link

codecov bot commented Jun 30, 2022

Codecov Report

Merging #2987 (65ad4b9) into main (36b37c4) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #2987   +/-   ##
=====================================
  Coverage   75.7%   75.7%           
=====================================
  Files        177     177           
  Lines      11813   11813           
=====================================
+ Hits        8943    8945    +2     
+ Misses      2636    2634    -2     
  Partials     234     234           
Impacted Files Coverage Δ
sdk/trace/provider.go 85.5% <100.0%> (ø)
exporters/jaeger/jaeger.go 91.1% <0.0%> (+0.8%) ⬆️

@Petrie
Copy link
Contributor Author

Petrie commented Jul 3, 2022

blocked by #2989

@dmathieu dmathieu added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jul 4, 2022
@MrAlias
Copy link
Contributor

MrAlias commented Oct 27, 2022

This looks to have been resolve a while ago as far as I can tell. We are currently at version 1.50.1 of golangci-lint: #3404

Closing as this looks superseded. Please reopen if this was in error.

@MrAlias MrAlias closed this Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants