enable gocognit linter #1037

grishagavrin · 2024-02-08T07:37:25Z

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Pull request type

Please check the type of change your PR introduces:

What is the current behavior?

Issue Number: #938

What is the new behavior?

Other information

First I load the files with the linter turned off to check the ci tests.
I have the linter enabled locally and with the last commit I enabled the linter in the remote repository.

codecov-commenter · 2024-02-08T07:40:15Z

Codecov Report

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

Comparison is base (c6e45cd) 68.48% compared to head (57a84ee) 68.72%.

Files	Patch %	Lines
internal/table/scanner/scanner.go	41.56%	87 Missing and 10 partials ⚠️
internal/decimal/decimal.go	0.00%	35 Missing ⚠️
log/table.go	83.65%	29 Missing and 5 partials ⚠️
sugar/path.go	43.24%	17 Missing and 4 partials ⚠️
log/sql.go	76.47%	8 Missing and 4 partials ⚠️
internal/xsql/dsn.go	66.66%	6 Missing and 2 partials ⚠️
retry/retry.go	27.27%	7 Missing and 1 partial ⚠️
internal/bind/params.go	80.00%	4 Missing and 2 partials ⚠️
internal/table/client.go	78.57%	2 Missing and 1 partial ⚠️
log/topic.go	97.18%	0 Missing and 2 partials ⚠️
... and 2 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1037      +/-   ##
==========================================
+ Coverage   68.48%   68.72%   +0.24%     
==========================================
  Files         252      252              
  Lines       25155    25420     +265     
==========================================
+ Hits        17228    17471     +243     
- Misses       7069     7093      +24     
+ Partials      858      856       -2

Flag	Coverage Δ
	`55.48% <60.96%> (+0.29%)`	⬆️
go-1.20.x	`68.57% <70.84%> (+0.30%)`	⬆️
go-1.21.x	`68.67% <70.84%> (+0.21%)`	⬆️
integration	`55.48% <60.96%> (+0.29%)`	⬆️
macOS	`38.34% <15.71%> (-0.32%)`	⬇️
ubuntu	`38.38% <15.71%> (-0.30%)`	⬇️
unit	`38.44% <15.71%> (-0.30%)`	⬇️
windows	`38.38% <15.71%> (-0.34%)`	⬇️
ydb-22.5	`55.00% <60.96%> (+0.17%)`	⬆️
ydb-23.1	`54.98% <59.31%> (+0.04%)`	⬆️
ydb-23.2	`55.16% <60.96%> (+0.34%)`	⬆️
ydb-23.3	`55.22% <59.31%> (+0.23%)`	⬆️

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.

…ith fixies in comments

…inter

…ocognit linter

…linter

…h off gocognit linter

…th off gocognit linter

…ests and upload with off gocognit linter

… and upload with off gocognit linter

…cks tests and upload with off gocognit linter

… linter

grishagavrin · 2024-02-10T10:21:41Z

I noted one comment //nolint:gocognit, because if I rewrite a function internal/cmd/gtrace.findGtraceGen in
for _, c := range v.List { if strings.Contains(strings.TrimPrefix(c.Text, "//"), "gtrace:gen") { if item == nil { item = &GenItem{} } } } the autotest breaks either tests / unit (1.21.x, windows)

asmyasnikov · 2024-03-07T12:28:41Z

CHANGELOG.md

I think this refactorings not valuable for users. Thats why no need to add lines to CHANGELOG

asmyasnikov · 2024-03-07T12:30:29Z

internal/backoff/backoff_test.go

I think no need to change test files for enabling gocongnit linter - this is no valuable for doing better source code
Can you add all test files to exclude-rules in .golangci.yaml config?

asmyasnikov · 2024-03-07T12:32:24Z

internal/stack/record.go

 }

+// findFileNameAndPkgPath finds the file, name and package path from the given file path, name and package path.
+func findFileNameAndPkgPath(file, name, pkgPath string) (string, string, string) {


need to unit test for newest func

asmyasnikov · 2024-03-07T12:33:07Z

internal/table/scanner/scanner.go

need to unit tests for newest func

asmyasnikov · 2024-03-07T12:34:22Z

internal/xsql/dsn.go

 }
+
+// bindTablePathPrefixInConnectorOptions binds table path prefix query transformers to a list of ConnectorOptions.
+func bindTablePathPrefixInConnectorOptions(queryTransformers []string) ([]ConnectorOption, error) {


queryTransformersToConnectorOptions?

asmyasnikov · 2024-03-07T12:34:33Z

internal/xsql/dsn.go

 }
+
+// bindTablePathPrefixInConnectorOptions binds table path prefix query transformers to a list of ConnectorOptions.
+func bindTablePathPrefixInConnectorOptions(queryTransformers []string) ([]ConnectorOption, error) {


need to unit test for newest func

asmyasnikov · 2024-03-07T12:35:20Z

log/driver.go

I think this file need to stay as is with nolint comment because your simple functions can be clashed in log package.
For example onInit func may clashed between driver log and table log and other

asmyasnikov · 2024-03-07T12:37:30Z

log/sql.go

I think this file need to stay as is with nolint comment because your simple functions can be clashed in log package.
For example onInit func may clashed between driver log and table log and other

asmyasnikov · 2024-03-07T12:37:45Z

log/table.go

I think this file need to stay as is with nolint comment because your simple functions can be clashed in log package.
For example onInit func may clashed between driver log and table log and other

asmyasnikov · 2024-03-07T12:37:54Z

log/topic.go

I think this file need to stay as is with nolint comment because your simple functions can be clashed in log package.
For example onInit func may clashed between driver log and table log and other

asmyasnikov · 2024-03-07T12:38:13Z

metrics/driver.go

I think this file need to stay as is with nolint comment because your simple functions can be clashed in log package.
For example onInit func may clashed between driver log and table log and other

asmyasnikov · 2024-03-07T12:39:24Z

retry/retry.go

+// with a panic recovery mechanism. If the `options.panicCallback` is specified, it
+// is invoked with the recovered value, and an error is returned with the recovered value wrapped
+// as the cause. This function returns the result of the `op` retry operation.
+func RecoveryCallbackWrapper(context context.Context, op retryOperation, options *retryOptions) (err error) {


public func not welcome

asmyasnikov · 2024-03-07T12:41:39Z

internal/cmd/gtrace/main.go

I think this file need to stay as is with nolint comment because findGtraceGen also commented with nolint word

asmyasnikov · 2024-03-07T12:42:04Z

internal/bind/params.go

 }
+
+// checkDriverNamedValue checks the driver.NamedValue and adds it to the params slice.
+func checkDriverNamedValue(


Need to test for newest func

asmyasnikov · 2024-03-07T12:43:38Z

internal/bind/params.go

 }
+
+// checkDriverNamedValue checks the driver.NamedValue and adds it to the params slice.
+func checkDriverNamedValue(


appendParams or tryAppendParams?

asmyasnikov · 2024-03-07T12:45:29Z

internal/decimal/decimal.go

 }

+// dotStringAnalysis performs analysis on a string representation of a decimal number.
+func dotStringAnalysis(


toBigInt or stringToBigInt?

asmyasnikov · 2024-03-07T12:45:55Z

internal/decimal/decimal.go

 }

+// dotStringAnalysis performs analysis on a string representation of a decimal number.
+func dotStringAnalysis(


need to add unit test for newest func

asmyasnikov · 2024-03-07T12:46:46Z

internal/decimal/decimal.go

+}
+
+// multipliedByTen multiplies the given big.Int value by 10 raised to the power of scale.
+func multipliedByTen(v *big.Int, scale uint32, neg bool) {


need to add unit test for newest func

asmyasnikov · 2024-03-07T12:50:21Z

internal/table/client.go

+
+		if timeout := c.config.DeleteTimeout(); timeout > 0 {
+			var cancel context.CancelFunc
+			createSessionCtx, cancel = xcontext.WithTimeout(closeSessionCtx, timeout)


createSessionCtx is not used

May be a bug in base version of code

asmyasnikov · 2024-03-07T12:52:48Z

internal/table/client.go

 }

+// onCloseSession is a closure function that takes a session and performs the actions to close it.
+func onCloseSession(ctx, createSessionCtx context.Context, c *Client) func(s *session) {


createSessionCtx and c are not used

asmyasnikov · 2024-03-07T12:58:46Z

internal/table/client.go

 }

+// onCloseSession is a closure function that takes a session and performs the actions to close it.
+func onCloseSession(ctx, createSessionCtx context.Context, c *Client) func(s *session) {


I think ctx need to move from source args to result
And your high-ordered func may looks like:

func closerWithTimeout(timeout time.Duration) func(ctx context.Context, closer closer.Closer) { return func(ctx context.Context, s *session) { if closer == nil { return } ctx = xcontext.WithoutDeadline(ctx) var cancel context.CancelFunc if timeout > 0 { ctx, cancel = xcontext.WithTimeout(ctx, timeout) } else { ctx, cancel = xcontext.WithCancel(ctx) } defer cancel() _ = closer.Close(ctx) } }

And this func can be tested with unit test

asmyasnikov · 2024-03-07T13:08:56Z

Summary of review:

Need to rebase to master
Need to add exclude-rules into .golangci.yaml config for:
- all tests
- packages log and metrics
Need to add unit tests for newest funcs
Minor issues see in full review

refactor internal/stack/record and upload with off gocognit linter

e39fb38

grishagavrin added 17 commits February 8, 2024 13:12

refactor sugar/path and upload with off gocognit linter

a33374f

refactor sugar/path.removeWithPrefix and upload with off gocognit linter

90dd596

refactor sugar/path.removeEntry and upload with off gocognit linter

3047f3f

refactor sugar/path.removeEntry and upload with off gocognit linter w…

f203aa4

…ith fixies in comments

refactor internal/xsql/dsn and upload with off gocognit linter

b5dcd3c

refactor internal/backoff/backoff_test and upload with off gocognit l…

efc29c9

…inter

refactor internal/decimal/decimal and upload with off gocognit linter

c6ef237

refactor internal/table/client and upload with off gocognit linter

9d5e827

refactor internal/table/retry_test and upload with off gocognit linter

3343652

refactor metrics/sql and upload with off gocognit linter

2ba5042

refactor metrics/driver and upload with off gocognit linter

3d56192

refactor log/sql and upload with off gocognit linter

7386724

refactor internal/table/scanner/scanner_test.go and upload with off g…

50d2eae

…ocognit linter

refactor log/topic and upload with off gocognit linter

1edb132

refactor internal/table/scanner/scanner and upload with off gocognit …

5ce51c4

…linter

refactor log/driver and upload with off gocognit linter

a2bf458

refactor internal/topic/topicwriterinternal/queue_test and upload wit…

14394d5

…h off gocognit linter

grishagavrin force-pushed the master branch from 1e0c0ca to f238f1d Compare February 9, 2024 07:54

refactor retry/retry and upload with off gocognit linter

c97da45

grishagavrin force-pushed the master branch from f238f1d to c97da45 Compare February 9, 2024 08:10

grishagavrin added 8 commits February 9, 2024 11:21

refactor internal/cmd/gtrace/main and upload with off gocognit linter

4765323

undo because check-codegen is falling

a3735b0

refactor internal/bind/params and upload with off gocognit linter

e06bc0e

refactor internal/cmd/gtrace/main manually checks tests and upload wi…

12265dc

…th off gocognit linter

refactor internal/cmd/gtrace/main.iterateOverWriter manually checks t…

0ed37d8

…ests and upload with off gocognit linter

refactor internal/cmd/gtrace/main.findGtraceGen manually checks tests…

3108b41

… and upload with off gocognit linter

refactor internal/cmd/gtrace/main.findCommentsWithGtrace manually che…

e7b2d6c

…cks tests and upload with off gocognit linter

refactor internal/cmd/gtrace/main.findCommentsWithGtrace manually che…

ec004e4

…cks tests and upload with off gocognit linter

grishagavrin added 4 commits February 10, 2024 12:17

refactor log/table manually checks tests and upload with off gocognit…

0b86a1c

… linter

refactor log/table manually checks tests and upload with off gocognit…

e81deb1

… linter

refactor log/table manually checks tests and upload with off gocognit…

bd278ba

… linter

gocognit linter on

57a84ee

asmyasnikov self-assigned this Feb 19, 2024

asmyasnikov reviewed Mar 7, 2024

View reviewed changes

CHANGELOG.md

Copy link

Member

asmyasnikov Mar 7, 2024

Choose a reason for hiding this comment

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

I think this refactorings not valuable for users. Thats why no need to add lines to CHANGELOG

asmyasnikov reviewed Mar 7, 2024

View reviewed changes

internal/table/scanner/scanner.go

Copy link

Member

asmyasnikov Mar 7, 2024

Choose a reason for hiding this comment

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

need to unit tests for newest func

asmyasnikov reviewed Mar 7, 2024

View reviewed changes

asmyasnikov force-pushed the master branch from 7be8c10 to c6a7a8b Compare March 11, 2024 12:34

Merge branch 'master' into master

d81f128

enable gocognit linter #1037

Are you sure you want to change the base?

enable gocognit linter #1037

Uh oh!

Conversation

grishagavrin commented Feb 8, 2024

Pull request type

What is the current behavior?

What is the new behavior?

Other information

Uh oh!

codecov-commenter commented Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

grishagavrin commented Feb 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asmyasnikov Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asmyasnikov Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asmyasnikov commented Mar 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Feb 8, 2024 •

edited

Loading

grishagavrin commented Feb 10, 2024 •

edited

Loading

asmyasnikov Mar 7, 2024 •

edited

Loading

asmyasnikov Mar 7, 2024 •

edited

Loading