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

Applying linting rules #354

Merged
merged 2 commits into from
Aug 27, 2021
Merged

Applying linting rules #354

merged 2 commits into from
Aug 27, 2021

Conversation

teivah
Copy link
Contributor

@teivah teivah commented Aug 26, 2021

Applying a bunch of linting rules:

  • boolean == true => boolean
  • Using the blank identifier to mark explicit the fact that an error is handled
  • Some errors that weren't handled
  • bytes.Compare(a, b) == 0 => bytes.Equal(a, b)
  • Using a constant when a string is repeated multiple times
  • Preallocating slices whenever possible for performance concerns
  • Avoiding useless conversions
  • time.Now().Sub(t) => time.Since(t)
  • Removing useless fmt.Sprintf
  • And other minor stuff

Also, one bug fix where this break was breaking the select, not the for loop

Please let me know what you guys think about it.

@CLAassistant
Copy link

CLAassistant commented Aug 26, 2021

CLA assistant check
All committers have signed the CLA.

@teivah teivah marked this pull request as ready for review August 26, 2021 21:56
@codecov
Copy link

codecov bot commented Aug 27, 2021

Codecov Report

Merging #354 (73bac93) into main (1e1267a) will decrease coverage by 0.18%.
The diff coverage is 34.62%.

❗ Current head 73bac93 differs from pull request most recent head a02bdde. Consider uploading reports for the commit a02bdde to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #354      +/-   ##
==========================================
- Coverage   54.36%   54.18%   -0.17%     
==========================================
  Files         102      102              
  Lines        5013     5013              
==========================================
- Hits         2725     2716       -9     
- Misses       2039     2044       +5     
- Partials      249      253       +4     
Impacted Files Coverage Δ
pkg/storage/local.go 0.00% <0.00%> (ø)
pkg/storage/tree/flamebearer.go 100.00% <ø> (ø)
pkg/storage/tree/treediff.go 98.48% <ø> (-0.02%) ⬇️
pkg/util/debug/reporter.go 0.00% <0.00%> (ø)
pkg/storage/storage.go 63.87% <26.67%> (-1.34%) ⬇️
pkg/storage/dimension/dimension.go 61.37% <50.00%> (ø)
pkg/storage/dict/dict.go 85.19% <100.00%> (ø)
pkg/storage/tree/serialize.go 70.09% <100.00%> (ø)
pkg/util/bytesize/bytesize.go 33.34% <100.00%> (-4.59%) ⬇️
pkg/agent/upstream/remote/remote.go 62.32% <0.00%> (-2.89%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5acc0b8...a02bdde. Read the comment docs.

@petethepig
Copy link
Member

These all look great, thank you @teivah for contributing these changes!

The only thing is, we shouldn't lint code at pkg/agent/pprof/* because it's copied from go repo and so I'm thinking in the future it will make it easier to port upstream changes if we don't change this code.

We do have an exclude in Makefile. Did you use revive / go lint manually to find these lint errors? I'm just trying to think how we can help people find out about the reason we don't lint code in pkg/agent/pprof/* easier.

@petethepig
Copy link
Member

Thank you @teivah! it looks great, merging.

@petethepig petethepig merged commit 785c839 into grafana:main Aug 27, 2021
@teivah
Copy link
Contributor Author

teivah commented Aug 27, 2021

Thanks @petethepig

The only thing is, we shouldn't lint code at pkg/agent/pprof/* because it's copied from go repo and so I'm thinking in the future it will make it easier to port upstream changes if we don't change this code.

Alright, reverted 👍

Regarding linting, I wasn't aware about revive so not sure how to exclude it. However, most of the things I tackled were coming from golangci-lint (which looks somehow to fill the same need as revive) and I executed the following command:

golangci-lint run --enable golint,gofmt,unparam,goconst,prealloc,stylecheck,unconvert --exclude-use-default=false --deadline=5m

@petethepig
Copy link
Member

@teivah ahh, I see, yeah, that makes sense.

korniltsev pushed a commit that referenced this pull request Jul 18, 2023
…mestamp

Set timestamp in profile, if not set
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