Fix deepsource: RVV-B0001 confusing naming of struct fields or methods #1844

kevindiu · 2022-11-18T06:28:17Z

Description:

This PR includes the following changes.

please review the `merge unexported function` items carefully, as they contains refactoring and may contains logical changes.

Rename unexported function if exported function with the same name exists
- e.g. search() -> doSearch(), open() -> doOpen()
- Files are:
  - hack/benchmark/assets/x1b/loader.go
  - internal/file/file.go
  - internal/net/grpc/client.go
  - internal/net/grpc/pool/pool.go
  - internal/net/http/transport/roundtrip.go
  - internal/safety/safety.go
  - internal/test/data/vector/gen.go
  - pkg/gateway/lb/handler/grpc/handler.go
Merge unexported function to the exported implementation (may contains logical changes)
- Files are:
  - internal/db/rdb/mysql/mysql.go
  - internal/worker/queue.go
Fix/refactor test implementation
- Some test case is merged from the unexported func test
Fix golangci warning
- Fix internal/net/http/transport/roundtrip.go and test to close the request/response body

Related Issue:

Versions:

Go Version: 1.19.2
Docker Version: 20.10.8
Kubernetes Version: 1.22.0
NGT Version: 1.14.8

Checklist:

I have read the CONTRIBUTING document and completed our CLA agreement.
I have checked open Pull Requests for the similar feature or fixes?

Special notes for your reviewer:

cloudflare-pages · 2022-11-18T06:29:05Z

Deploying with Cloudflare Pages

Latest commit:	`182c2ff`
Status:	✅ Deploy successful!
Preview URL:	https://f88a5ea2.vald.pages.dev
Branch Preview URL:	https://refactor-rvv-b0001-confusing.vald.pages.dev

View logs

codecov · 2022-11-18T07:04:14Z

Codecov Report

Base: 30.55% // Head: 30.54% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (182c2ff) compared to base (88f19ab).
Patch coverage: 44.44% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1844      +/-   ##
==========================================
- Coverage   30.55%   30.54%   -0.01%     
==========================================
  Files         367      367              
  Lines       33830    33821       -9     
==========================================
- Hits        10337    10331       -6     
+ Misses      23082    23079       -3     
  Partials      411      411

Impacted Files	Coverage Δ
hack/benchmark/assets/x1b/loader.go	`0.00% <0.00%> (ø)`
internal/net/grpc/client.go	`0.00% <0.00%> (ø)`
internal/net/grpc/pool/pool.go	`0.00% <0.00%> (ø)`
pkg/gateway/lb/handler/grpc/handler.go	`0.00% <0.00%> (ø)`
internal/file/file.go	`11.93% <14.28%> (ø)`
internal/safety/safety.go	`92.85% <66.66%> (ø)`
internal/db/rdb/mysql/mysql.go	`94.54% <100.00%> (-0.04%)`	⬇️
internal/net/http/transport/roundtrip.go	`88.46% <100.00%> (+0.70%)`	⬆️
internal/test/data/vector/gen.go	`92.85% <100.00%> (-0.55%)`	⬇️
internal/worker/queue.go	`98.68% <100.00%> (-1.32%)`	⬇️
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

internal/test/data/vector/gen.go

internal/net/http/transport/roundtrip.go

internal/net/http/transport/roundtrip_test.go

github-actions · 2022-11-21T03:42:45Z

internal/db/rdb/mysql/mysql_test.go

@@ -2793,10 +2793,10 @@ func Test_mySQLClient_SetVectors(t *testing.T) {
 	}
 }

-func Test_mySQLClient_deleteVector(t *testing.T) {
+func Test_mySQLClient_DeleteVector(t *testing.T) {


🚫 [golangci] _{reported by reviewdog 🐶}
Function name: Test_mySQLClient_DeleteVector, Cyclomatic Complexity: 14, Halstead Volume: 15129.92, Maintainability Index: 8 (maintidx)

Since it is test function, it is hard to avoid complex implementation.
I will ignore this warning.
Please let me know it you have any other comment for this warning. :)

internal/net/grpc/pool/pool.go

kevindiu · 2022-11-21T05:51:04Z

internal/net/grpc/pool/pool.go

-}
-
-func (p *pool) get(retry uint64) (*ClientConn, bool) {
-	if retry <= 0 || retry > math.MaxUint64-p.Len() || p.Len() <= 0 {


Do anyone know what is the checking retry > math.MaxUint64-p.Len() means here? I have omitted it on my refactoring.

@kpango do you know what is the meaning of this logic?
I wanted to refactor it but I can't understand what it means, so I couldn't complete this refactoring for now.

internal/net/grpc/pool/pool.go

internal/test/data/vector/gen.go

Signed-off-by: kevindiu <kevindiujp@gmail.com>

…t-fields-or-methods

internal/test/data/vector/gen.go

Signed-off-by: kevindiu <kevindiujp@gmail.com>

internal/test/data/vector/gen.go

…t-fields-or-methods

github-actions bot added area/gateway/lb area/internal team/set SET team size/L type/refactoring priority/low labels Nov 18, 2022

github-actions bot reviewed Nov 18, 2022

View reviewed changes

internal/test/data/vector/gen.go Outdated Show resolved Hide resolved

internal/net/http/transport/roundtrip.go Show resolved Hide resolved

internal/net/http/transport/roundtrip_test.go Show resolved Hide resolved

kevindiu changed the title ~~[WIP] Refactor/rvv b0001/confusing naming of struct fields or methods~~ Fix deepsource: FRVV-B0001 confusing naming of struct fields or methods Nov 21, 2022

kevindiu changed the title ~~Fix deepsource: FRVV-B0001 confusing naming of struct fields or methods~~ Fix deepsource: RVV-B0001 confusing naming of struct fields or methods Nov 21, 2022

github-actions bot added size/XXL and removed size/L labels Nov 21, 2022

github-actions bot reviewed Nov 21, 2022

View reviewed changes

kevindiu commented Nov 21, 2022

View reviewed changes

kevindiu marked this pull request as ready for review November 21, 2022 05:52

kevindiu requested review from vankichi, datelier and hlts2 November 21, 2022 05:59

kevindiu commented Nov 21, 2022

View reviewed changes

internal/net/grpc/pool/pool.go Outdated Show resolved Hide resolved

kevindiu changed the title ~~Fix deepsource: RVV-B0001 confusing naming of struct fields or methods~~ [WIP] Fix deepsource: RVV-B0001 confusing naming of struct fields or methods Nov 21, 2022

kevindiu marked this pull request as draft November 21, 2022 06:54

hlts2 previously approved these changes Nov 21, 2022

View reviewed changes

github-actions bot reviewed Nov 21, 2022

View reviewed changes

internal/test/data/vector/gen.go Outdated Show resolved Hide resolved

hlts2 self-requested a review November 21, 2022 07:54

kevindiu added 5 commits November 24, 2022 14:03

remove pop from queue

ad9a04c

Signed-off-by: kevindiu <kevindiujp@gmail.com>

fix part of confusing name

373007e

Signed-off-by: kevindiu <kevindiujp@gmail.com>

fix confusing name

6a0718f

Signed-off-by: kevindiu <kevindiujp@gmail.com>

rename search to doSearch

f3980a1

Signed-off-by: kevindiu <kevindiujp@gmail.com>

fix comment

66dadb9

Signed-off-by: kevindiu <kevindiujp@gmail.com>

Update internal/net/grpc/pool/pool.go

91e4db3

kevindiu dismissed hlts2’s stale review via 91e4db3 November 24, 2022 05:07

kevindiu force-pushed the refactor/RVV-B0001/confusing-naming-of-struct-fields-or-methods branch from 6b68077 to 91e4db3 Compare November 24, 2022 05:07

github-actions bot added size/XL and removed size/XL labels Nov 24, 2022

kevindiu added 2 commits November 24, 2022 14:11

revert some changes

e397736

Signed-off-by: kevindiu <kevindiujp@gmail.com>

Update internal/test/data/vector/gen.go

9db978c

github-actions bot added size/XL and removed size/XL labels Nov 24, 2022

Merge branch 'main' into refactor/RVV-B0001/confusing-naming-of-struc…

bc42a54

…t-fields-or-methods

github-actions bot reviewed Nov 24, 2022

View reviewed changes

internal/test/data/vector/gen.go Outdated Show resolved Hide resolved

github-actions bot added size/XL and removed size/XL labels Nov 24, 2022

revert some changes

b336b38

Signed-off-by: kevindiu <kevindiujp@gmail.com>

github-actions bot added the size/XL label Nov 24, 2022

github-actions bot reviewed Nov 24, 2022

View reviewed changes

internal/test/data/vector/gen.go Outdated Show resolved Hide resolved

github-actions bot removed the size/XL label Nov 24, 2022

kevindiu changed the title ~~[WIP] Fix deepsource: RVV-B0001 confusing naming of struct fields or methods~~ Fix deepsource: RVV-B0001 confusing naming of struct fields or methods Nov 24, 2022

kevindiu marked this pull request as ready for review November 24, 2022 05:47

github-actions bot reviewed Nov 24, 2022

View reviewed changes

internal/test/data/vector/gen.go Outdated Show resolved Hide resolved

github-actions bot reviewed Nov 24, 2022

View reviewed changes

internal/test/data/vector/gen.go Outdated Show resolved Hide resolved

datelier approved these changes Nov 24, 2022

View reviewed changes

Merge branch 'main' into refactor/RVV-B0001/confusing-naming-of-struc…

182c2ff

…t-fields-or-methods

vankichi approved these changes Nov 24, 2022

View reviewed changes

github-actions bot added size/XL and removed size/XL labels Nov 24, 2022

vankichi merged commit 1fbd3bb into main Nov 25, 2022

vankichi deleted the refactor/RVV-B0001/confusing-naming-of-struct-fields-or-methods branch November 25, 2022 00:29

kpango mentioned this pull request Jan 19, 2023

[minor] Release v1.7.0 #1926

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix deepsource: RVV-B0001 confusing naming of struct fields or methods #1844

Fix deepsource: RVV-B0001 confusing naming of struct fields or methods #1844

kevindiu commented Nov 18, 2022 •

edited

Loading

cloudflare-pages bot commented Nov 18, 2022 •

edited

Loading

codecov bot commented Nov 18, 2022 •

edited

Loading

github-actions bot Nov 21, 2022

kevindiu Nov 21, 2022

kevindiu Nov 21, 2022

kevindiu Nov 21, 2022

Fix deepsource: RVV-B0001 confusing naming of struct fields or methods #1844

Fix deepsource: RVV-B0001 confusing naming of struct fields or methods #1844

Conversation

kevindiu commented Nov 18, 2022 • edited Loading

Description:

please review the merge unexported function items carefully, as they contains refactoring and may contains logical changes.

Related Issue:

Versions:

Checklist:

Special notes for your reviewer:

cloudflare-pages bot commented Nov 18, 2022 • edited Loading

Deploying with Cloudflare Pages

codecov bot commented Nov 18, 2022 • edited Loading

Codecov Report

github-actions bot Nov 21, 2022

Choose a reason for hiding this comment

kevindiu Nov 21, 2022

Choose a reason for hiding this comment

kevindiu Nov 21, 2022

Choose a reason for hiding this comment

kevindiu Nov 21, 2022

Choose a reason for hiding this comment

kevindiu commented Nov 18, 2022 •

edited

Loading

please review the `merge unexported function` items carefully, as they contains refactoring and may contains logical changes.

cloudflare-pages bot commented Nov 18, 2022 •

edited

Loading

codecov bot commented Nov 18, 2022 •

edited

Loading