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 Lint Rule: line-length-limit #5540

Closed
wants to merge 4 commits into from

Conversation

varshith257
Copy link
Contributor

@varshith257 varshith257 commented Jun 5, 2024

Which problem is this PR solving?

Description of the changes

  • Refactored the files to fix the lint of line-length limit

How was this change tested?

  • make lint

Checklist

Signed-off-by: Vamshi Maskuri <[email protected]>
@varshith257 varshith257 requested a review from a team as a code owner June 5, 2024 08:14
@varshith257 varshith257 requested a review from pavolloffay June 5, 2024 08:14
@varshith257
Copy link
Contributor Author

@yurishkuro I have used the golines library to fix it with the following command:

golines -m 80 -w .

It has fixed most of it but still emits the lint errors for comments. Can comments also need fixes or add ignore?

Copy link

codecov bot commented Jun 5, 2024

Codecov Report

Attention: Patch coverage is 97.36973% with 53 lines in your changes missing coverage. Please review.

Project coverage is 96.18%. Comparing base (52473e7) to head (b33e98c).

Files Patch % Lines
cmd/anonymizer/app/query/query.go 0.00% 18 Missing ⚠️
cmd/collector/app/collector.go 76.66% 7 Missing ⚠️
cmd/ingester/app/consumer/consumer.go 88.23% 6 Missing ⚠️
cmd/query/app/server.go 92.68% 4 Missing and 2 partials ⚠️
cmd/ingester/app/builder/builder.go 16.66% 5 Missing ⚠️
...ger/internal/exporters/storageexporter/exporter.go 42.85% 4 Missing ⚠️
cmd/query/app/static_handler.go 93.75% 4 Missing ⚠️
cmd/agent/app/testutils/mock_grpc_collector.go 0.00% 1 Missing ⚠️
cmd/collector/app/server/test.go 50.00% 1 Missing ⚠️
pkg/cassandra/config/config.go 90.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5540      +/-   ##
==========================================
- Coverage   96.20%   96.18%   -0.02%     
==========================================
  Files         327      327              
  Lines       16007    18719    +2712     
==========================================
+ Hits        15399    18005    +2606     
- Misses        432      538     +106     
  Partials      176      176              
Flag Coverage Δ
badger_v1 8.07% <0.85%> (+0.01%) ⬆️
badger_v2 1.77% <1.69%> (-0.17%) ⬇️
cassandra-3.x-v1 16.50% <6.81%> (+0.05%) ⬆️
cassandra-3.x-v2 1.70% <1.69%> (-0.16%) ⬇️
cassandra-4.x-v1 16.50% <6.81%> (+0.05%) ⬆️
cassandra-4.x-v2 1.70% <1.69%> (-0.16%) ⬇️
elasticsearch-7-v1 19.07% <1.42%> (+0.18%) ⬆️
elasticsearch-8-v1 19.24% <1.42%> (+0.15%) ⬆️
elasticsearch-8-v2 19.24% <1.42%> (+0.15%) ⬆️
grpc_v1 9.13% <2.55%> (-0.34%) ⬇️
grpc_v2 7.03% <5.08%> (-0.50%) ⬇️
kafka 9.85% <30.39%> (+0.08%) ⬆️
opensearch-1-v1 19.14% <1.42%> (+0.19%) ⬆️
opensearch-2-v1 19.14% <1.42%> (+0.19%) ⬆️
opensearch-2-v2 19.15% <1.42%> (+0.20%) ⬆️
unittests 93.93% <96.87%> (-0.15%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yurishkuro
Copy link
Member

Sorry, but I am not going to review 519 files. There was a clear comment in the config that the default length is not acceptable to us.

@varshith257
Copy link
Contributor Author

varshith257 commented Jun 5, 2024

Yes, the config contains 80 is too small :)

Okay, will divide it into multiple PRS. What could be an acceptable line length limit? I am thinking of 120 which I have seen in a Google article on the recommended line length limit for Golang or any suggestions?

@yurishkuro
Copy link
Member

I am thinking of 120 which I have seen in a Google article on the recommended line length limit for Golang or any suggestions?

How many breaks would be detected with 120?

@varshith257
Copy link
Contributor Author

varshith257 commented Jun 7, 2024

Ok, I have made changes and here are about 80 and 120 lll changes will reflect in the codebase:

80 lll - 18k+ lines and 519 files changed
120 lll - 2708 lines and 213 files changed

@yurishkuro Maybe we can go with 120. WDYT?

@yurishkuro
Copy link
Member

213 files is still too many, not worth the effort.

@varshith257
Copy link
Contributor Author

@yurishkuro For 150 there are 81 files changed

@yurishkuro
Copy link
Member

still not worth it. I don't want to spend an hour reviewing such changes that provide little benefit. I usually try to fix long lines when making changes in the code, as a side effect.

@varshith257
Copy link
Contributor Author

ok! Closing

@varshith257 varshith257 closed this Jun 7, 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.

2 participants