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

Adressing linter issues in netext, netext/httpext packages #3484

Merged
merged 5 commits into from
Dec 12, 2023

Conversation

olegbespalov
Copy link
Contributor

@olegbespalov olegbespalov commented Dec 1, 2023

What?

This PR addresses the linter issues in the netext package.

It's better to do a review by commits. I've tried to combine them for the impact.

The first commit is applying simple changes.

Everything else deserves a more focused look since it also contains some refactorings.

Note: one linter issue is still there:

lib/netext/httpext/request.go:122:2                nestif             `if preq.Body != nil` has complex nested blocks (complexity: 6)

Since the function it deserves a separate PR, it's more complicated.

// MakeRequest makes http request for tor the provided ParsedHTTPRequest.
//
// TODO: split apart...
//
//nolint:cyclop, gocyclo, funlen, gocognit
func MakeRequest(ctx context.Context, state *lib.State, preq *ParsedHTTPRequest) (*Response, error) {

Also, grpcext package's linter issues have yet to be touched since I'm planning to address them in #3152

Why?

As we agreed, we have to fix the linter issues presented in the master branch.

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

Part of #769

@olegbespalov olegbespalov added this to the v0.49.0 milestone Dec 1, 2023
@olegbespalov olegbespalov self-assigned this Dec 1, 2023
@github-actions github-actions bot requested review from joanlopez and oleiade December 1, 2023 09:09
@olegbespalov olegbespalov changed the title Chore/lintefixe netext Adressing linter issues in netext, netext/httpext packages Dec 1, 2023
@codecov-commenter
Copy link

codecov-commenter commented Dec 1, 2023

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (e23ebec) 73.16% compared to head (c8ccf57) 73.18%.
Report is 1 commits behind head on master.

❗ Current head c8ccf57 differs from pull request most recent head 4970d55. Consider uploading reports for the commit 4970d55 to get more accurate results

Files Patch % Lines
lib/netext/httpext/compression.go 75.00% 5 Missing ⚠️
lib/netext/httpext/tracer.go 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3484      +/-   ##
==========================================
+ Coverage   73.16%   73.18%   +0.01%     
==========================================
  Files         267      267              
  Lines       20076    20076              
==========================================
+ Hits        14689    14692       +3     
+ Misses       4471     4470       -1     
+ Partials      916      914       -2     
Flag Coverage Δ
ubuntu 73.13% <76.00%> (+0.01%) ⬆️
windows 73.04% <76.00%> (+0.01%) ⬆️

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.

mstoykov
mstoykov previously approved these changes Dec 7, 2023
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

LGTM, but instead of sparkling nolint:revive it is just better to replace the unused arguments name with _

@olegbespalov olegbespalov added the breaking change for PRs that need to be mentioned in the breaking changes section of the release notes label Dec 11, 2023
@olegbespalov olegbespalov merged commit 91ef773 into master Dec 12, 2023
21 checks passed
@olegbespalov olegbespalov deleted the chore/lintefixe-netext branch December 12, 2023 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change for PRs that need to be mentioned in the breaking changes section of the release notes maintenance refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants