Enable funlen linter (part 2) #1171

brojeg · 2024-03-27T19:29:29Z

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: 934

What is the new behavior?

Other information

brojeg · 2024-03-27T19:29:52Z

@rekby Please review.

asmyasnikov · 2024-03-28T02:14:06Z

internal/stack/record.go

+	return name, fileName
+}
+
+func parseFunctionName(name string) (pkgPath, pkgName, structName, funcName string, lambdas []string) {


Can you add a unit tests for newest functions?

codecov-commenter · 2024-03-28T20:27:29Z

Codecov Report

Attention: Patch coverage is 66.66667% with 42 lines in your changes are missing coverage. Please review.

Project coverage is 41.08%. Comparing base (b7a9b51) to head (013a60c).
Report is 47 commits behind head on master.

Files	Patch %	Lines
internal/decimal/decimal.go	51.19%	35 Missing and 6 partials ⚠️
retry/retry.go	92.30%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1171      +/-   ##
==========================================
+ Coverage   40.80%   41.08%   +0.28%     
==========================================
  Files         309      310       +1     
  Lines       32747    32959     +212     
==========================================
+ Hits        13361    13540     +179     
- Misses      18959    18986      +27     
- Partials      427      433       +6

Flag	Coverage Δ
go-1.21.x	`43.29% <66.66%> (+0.35%)`	⬆️
go-1.22.x	`41.07% <66.66%> (+0.28%)`	⬆️
macOS	`40.60% <68.46%> (+0.30%)`	⬆️
ubuntu	`40.58% <68.46%> (+0.28%)`	⬆️
unit	`41.08% <66.66%> (+0.28%)`	⬆️
windows	`41.05% <66.66%> (+0.25%)`	⬆️

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.

rekby

Hello, thanks for the work!
it is very good for your work for code style.

I see new part better, for example you add linter skip for one of functions, which really doesn't need to split.

I have some for the before merge it.

internal/decimal/decimal.go

rekby · 2024-03-29T13:18:19Z

internal/decimal/decimal.go

+			*scale--
+		} else if !dot {
+			if *integral == 0 {
+				remaining += string(c)


Every addition to string has linear complexity and re-allocate memory.
Use strings.Builder{} for build the string.

rekby · 2024-03-29T13:19:47Z

internal/decimal/decimal.go

+	}
+
+	integral := precision - scale
+	s, err := parseNumber(s, v, &integral, &scale)


What mean integral and scale after return from parseNumber?

Do we need pass the values by pointers?

rekby · 2024-03-29T13:25:25Z

internal/decimal/decimal.go

+		return syntaxError(s)
+	}
+	plus := c > '5'
+	if !plus && c == '5' {


no need check plus, you can check c == '5' only

rekby · 2024-03-29T13:28:12Z

internal/decimal/decimal.go

-		return v.Set(nan), nil
+func shouldRoundUp(v *big.Int, s string) bool {
+	var x big.Int
+	plus := x.And(v, big.NewInt(1)).Cmp(big.NewInt(0)) != 0


use one and zero variables for prevent new allocation for every function call

rekby · 2024-03-29T14:55:06Z

internal/stack/record.go

+	return name, fileName
+}
+
+func parseFunctionName(name string) (pkgPath, pkgName, structName, funcName string, lambdas []string) {


A lot of results, especially with same type is difficult to understand in code and use.
What about return a structure?

retry/retry.go

rekby · 2024-03-29T15:00:16Z

retry/retry.go


 			if err == nil {
 				return nil
 			}

 			if ctxErr := ctx.Err(); ctxErr != nil {
-				return xerrors.WithStackTrace(


no need extract simple error formatting

retry/retry.go

rekby · 2024-04-01T13:19:16Z

Thanks, it seems good.

I need some time to check PR for logic equals, because it was not only split code to smaller parts, but refactor too.

brojeg · 2024-04-09T17:35:37Z

Is there any news on this PR?

rekby · 2024-04-10T06:44:19Z

Is there any news on this PR?

Thanks for remind, I will plan to see the PR today.

rekby

Good job, thanks.

See for one additional method for test equals of old and new function while refactoring.

rekby · 2024-04-01T13:04:39Z

internal/decimal/decimal.go

-			v.Set(inf)
+
+	if c >= '5' {
+		if c > '5' || shouldRoundUp(v, s[1:]) {


in prev version it was shouldRoundUp(v, s), why che call changed to shouldRoundUp(v, s[1:]) and start check from next symbol?

reverted the change.

rekby · 2024-04-10T09:36:38Z

internal/decimal/decimal.go

+	return v, nil
+}
+
+func SetSpecialValue(v *big.Int, s string) bool {
 	neg := s[0] == '-'


it will be good to use function parseSign here

Used existing function parseSign

Now the function setSpecialValue do many things, instead only parse special values.

Now it:

Detect negative values

Cut sign from string

And it has interface with difficult to understand: it change input "v" and return it.
Usually function return new value without change input, rarer change input without return it.

Change and return used for chaining calls within builder - for you can call Obj.Method1().Method2().Method3().... with one line.

please return separate function parseSign function for detect and cut off sign and reuse the function here and in parseNumber.

Or detect and cut sign before call setSpecialValue (better parseSpecialValue) and pass the sign to the function, then to parseNumber if you won't check the sign twice.

P.S. i think about check the sign twice better - it is very small check, but it more understandable to read, then many parameters.

rekby · 2024-04-10T10:19:45Z

internal/decimal/decimal.go

@@ -95,129 +95,177 @@ func Parse(s string, precision, scale uint32) (*big.Int, error) {
 		return v, nil


New version has difference with old version of Parse in some cases.

please add fuzzing test for check equals of old and new Parse function results:

func FuzzParse(f *testing.F) { f.Fuzz(func(t *testing.T, s string, precision, scale uint32) { expectedRes, expectedErr := oldParse(s, precision, scale) res, err := Parse(s, precision, scale) if expectedErr == nil { require.Equal(t, expectedRes, res) } else { require.Error(t, err) } }) }

Where oldParse is copy of Parse from main branch.
Now result has differences at arguments "100", 0, 0.

You can past oldParse near the test in decimal_test.go, I will remove it from the branch after merge.

You can add better fuzz-test for better test long numbers instead of random strings only.

You can run fuzzing by command:

go test -fuzz=FuzzParse

https://go.dev/doc/security/fuzz/

added the provided test function FuzzParse and oldParse function to decimal_test.go

rekby

Test FuzzParse is failed.

=== RUN   FuzzParse
--- FAIL: FuzzParse (27.30s)
=== RUN   FuzzParse/1279e0a916766c26
    decimal_test.go:366: 
        	Error Trace:	/Users/rekby/projects/yandex/ydb-go-sdk-workspace/ydb-go-sdk/internal/decimal/decimal_test.go:366
        	            				/Users/rekby/gosdk/go1.22.0/src/reflect/value.go:596
        	            				/Users/rekby/gosdk/go1.22.0/src/reflect/value.go:380
        	            				/Users/rekby/gosdk/go1.22.0/src/testing/fuzz.go:335
        	Error:      	Not equal: 
        	            	expected: 100000000000000000000000000000000000
        	            	actual  : 0
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -2,6 +2,3 @@
        	            	  neg: (bool) false,
        	            	- abs: (big.nat) (len=2) {
        	            	-  (big.Word) 3136633892082024448,
        	            	-  (big.Word) 5421010862427522
        	            	- }
        	            	+ abs: (big.nat) <nil>
        	            	 })
        	Test:       	FuzzParse/1279e0a916766c26
    --- FAIL: FuzzParse/1279e0a916766c26 (27.30s)

Expected :100000000000000000000000000000000000
Actual   :0

With same input as in my previous comment:

go test fuzz v1
string("100")
uint32(0)
uint32(0)

It mean that result of old and new functions have differences for string "100", zero scale and zero precision.

rekby

Hello, good work, thanks!
I don't found errors now.

It need only small cosmetic fix and resolve conflicts before merge

rekby · 2024-04-16T10:31:22Z

internal/decimal/decimal.go

+	return v, nil
+}
+
+func SetSpecialValue(v *big.Int, s string) bool {
 	neg := s[0] == '-'


Now the function setSpecialValue do many things, instead only parse special values.

Now it:

Detect negative values

Cut sign from string

And it has interface with difficult to understand: it change input "v" and return it.
Usually function return new value without change input, rarer change input without return it.

Change and return used for chaining calls within builder - for you can call Obj.Method1().Method2().Method3().... with one line.

please return separate function parseSign function for detect and cut off sign and reuse the function here and in parseNumber.

Or detect and cut sign before call setSpecialValue (better parseSpecialValue) and pass the sign to the function, then to parseNumber if you won't check the sign twice.

P.S. i think about check the sign twice better - it is very small check, but it more understandable to read, then many parameters.

brojeg · 2024-04-16T17:25:39Z

I'm not quite sure how to resolve the particular merge conflict.
The particular code that needs to be merged in extractLambdas function exists under parseFunctionName function in my PR.

rekby · 2024-04-17T07:58:10Z

I'm not quite sure how to resolve the particular merge conflict. The particular code that needs to be merged in extractLambdas function exists under parseFunctionName function in my PR.

You can't merge it "automatically", because you have refactored the code.
You should see what mean changes in master branch, then re-implement the changes at own code. Then remove master's code block on merge.

brojeg · 2024-04-17T15:32:07Z

@rekby please review now.

rekby

Thanks for great work!

rekby

Attention:

I have pushed commit to your branch for resolve merge conflict with master. Pull the branch before work with the code.

rekby · 2024-04-23T15:51:15Z

internal/stack/record.go

 	if i := strings.LastIndex(file, "/"); i > -1 {
-		file = file[i+1:]
+		fileName = file[i+1:]
+	} else {


Once more point:
record.go was 100% coveraged before refactor and 98.8% after.

One branch of the code not tested: point when file doesn't contain /.
Add test for this please and PR will be merge.

I fixed the issue by replace the code by function from standard library - for extrace file name from path.

refactor funlen linter

0b8daa4

asmyasnikov reviewed Mar 28, 2024

View reviewed changes

added missing tests for the refactored functions

ed24a6e

rekby requested changes Mar 29, 2024

View reviewed changes

rekby self-assigned this Mar 29, 2024

refactor after PR review

bacf99b

rekby added the no changelog label Apr 1, 2024

rekby requested changes Apr 10, 2024

View reviewed changes

refactor after review

63a94fd

rekby requested changes Apr 11, 2024

View reviewed changes

Gleb Brozhe added 3 commits April 11, 2024 11:33

fixes after Fuzzing test

afdd3e8

added test for the specific scenario from the PR

922b301

an attempt to resolve merge conflict

07d827e

brojeg requested a review from rekby April 15, 2024 14:39

rekby requested changes Apr 16, 2024

View reviewed changes

refactor after PR review

8a1e868

Gleb Brozhe added 2 commits April 17, 2024 08:25

attempt to fix merge conflict

cc8c811

attempt to resolve merge conflict

757f14f

merging with main

0134ea4

brojeg requested a review from rekby April 22, 2024 14:44

rekby approved these changes Apr 23, 2024

View reviewed changes

Merge branch 'master' into master

a056f81

rekby requested changes Apr 23, 2024

View reviewed changes

fix coverage

013a60c

rekby approved these changes Apr 23, 2024

View reviewed changes

rekby merged commit d063e33 into ydb-platform:master Apr 24, 2024
35 of 36 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable funlen linter (part 2) #1171

Enable funlen linter (part 2) #1171

brojeg commented Mar 27, 2024

brojeg commented Mar 27, 2024

asmyasnikov Mar 28, 2024

brojeg Mar 29, 2024

codecov-commenter commented Mar 28, 2024 •

edited

Loading

rekby left a comment

rekby Mar 29, 2024

rekby Mar 29, 2024

rekby Mar 29, 2024

rekby Mar 29, 2024

rekby Mar 29, 2024

rekby Mar 29, 2024

rekby commented Apr 1, 2024

brojeg commented Apr 9, 2024

rekby commented Apr 10, 2024

rekby left a comment

rekby Apr 1, 2024

brojeg Apr 10, 2024

rekby Apr 10, 2024

brojeg Apr 10, 2024

rekby Apr 16, 2024

rekby Apr 10, 2024

brojeg Apr 10, 2024

rekby left a comment

rekby left a comment

rekby Apr 16, 2024

brojeg commented Apr 16, 2024

rekby commented Apr 17, 2024

brojeg commented Apr 17, 2024 •

edited

Loading

rekby left a comment

rekby left a comment

rekby Apr 23, 2024

rekby Apr 23, 2024

		@@ -95,129 +95,177 @@ func Parse(s string, precision, scale uint32) (*big.Int, error) {
		return v, nil

Enable funlen linter (part 2) #1171

Enable funlen linter (part 2) #1171

Conversation

brojeg commented Mar 27, 2024

Pull request type

What is the current behavior?

What is the new behavior?

Other information

brojeg commented Mar 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 28, 2024 • edited Loading

Codecov Report

rekby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rekby commented Apr 1, 2024

brojeg commented Apr 9, 2024

rekby commented Apr 10, 2024

rekby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rekby left a comment

Choose a reason for hiding this comment

rekby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brojeg commented Apr 16, 2024

rekby commented Apr 17, 2024

brojeg commented Apr 17, 2024 • edited Loading

rekby left a comment

Choose a reason for hiding this comment

rekby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 28, 2024 •

edited

Loading

brojeg commented Apr 17, 2024 •

edited

Loading