ultraware / funlen

MIT License

37 stars 8 forks source link

Revise funlen default behavior/configurations #2

Closed hollowaykeanho closed 5 years ago

hollowaykeanho commented 5 years ago

Issue Requirements

Thank you for creating the issue!

Please include the following information:

Version of golangci-lint: golangci-lint --version (or git commit if you don't use binary distribution) golangci-lint has version v1.19.1
Config file: cat .golangci.yml Not applicable

Go environment: go version && go env

GO111MODULE=""
GOARCH="amd64"
GOBIN="/home/u0/bin"
GOCACHE="/home/u0/.cache/go-build"
GOENV="/home/u0/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/u0"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/u0/Documents/gosandbox/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build509950643=/tmp/go-build -gno-record-gcc-switches"

Verbose output of running: golangci-lint run -v Not applicable

Problem

I getting a lot of funlen false positive and this linter's configurations make no sense. The command I use on daily basis is:

$ golangci-lint run --color never --enable-all "$dirpath"

Now I have to alter it to either:

$ golangci-lint run --color never --enable-all --disable=funlen "$dirpath"
OR
create a configuration file that set `funlen` instead of
using golang-lint off the shelves.
OR
commit new codes all over the places with //nolint:funlen

Reason for Changes

package float

//nolint:funlen
func testENDECScenarios() []testENDECScenario {
    return []testENDECScenario{
        {
            UID:      1,
            TestType: testToString,
            Description: `
Float.ToString is able to convert:
1. positive float32
2. positive accuracy
3. to Normal format
`,
            Switches: map[string]bool{
                useNormalFormat:     true,
                usePositiveFloat32:  true,
                usePositiveAccuracy: true,
            },
        }, {
            UID:      2,
            TestType: testToString,
            Description: `
Float.ToString is able to convert:
1. zero float32
2. positive accuracy
3. to Normal format
`,
            Switches: map[string]bool{
                useNormalFormat:     true,
                useZeroFloat32:      true,
                usePositiveAccuracy: true,
            },
        }, {
            UID:      3,
            TestType: testToString,
            Description: `
Float.ToString is able to convert:
1. negative good float32
2. positive accuracy
3. to Normal format
`,
            Switches: map[string]bool{
                useNormalFormat:     true,
                useNegativeFloat32:  true,
                usePositiveAccuracy: true,
            },
        }, {
            UID:      4,
            TestType: testToString,
            Description: `
Float.ToString is able to convert:
1. none
2. positive accuracy
3. to Normal format
`,
            Switches: map[string]bool{
                useNormalFormat:     true,
                usePositiveAccuracy: true,
            },
        }, {
            UID:      5,
            TestType: testToString,
            Description: `
Float.ToString is able to convert:
1. positive float32
2. zero accuracy
3. to Normal format
`,
            Switches: map[string]bool{
                useNormalFormat:    true,
                usePositiveFloat32: true,
                useZeroAccuracy:    true,
            },
        }, {
            UID:      6,
            TestType: testToString,
            Description: `
Float.ToString is able to convert:
1. positive float32
2. negative accuracy
3. to Normal format
`,
            Switches: map[string]bool{
                useNormalFormat:     true,
                usePositiveFloat32:  true,
                useNegativeAccuracy: true,
            },
        }, {
            UID:      7,
            TestType: testToString,
            Description: `
Float.ToString is able to convert:
1. positive float32
2. positive accuracy
3. to Scientific format
`,
            Switches: map[string]bool{
                useScientificFormat: true,
                usePositiveFloat32:  true,
                usePositiveAccuracy: true,
            },
        }, {
            UID:      8,
            TestType: testToString,
            Description: `
Float.ToString is able to convert:
1. zero float32
2. positive accuracy
3. to Scientific format
`,
            Switches: map[string]bool{
                useScientificFormat: true,
                useZeroFloat32:      true,
                usePositiveAccuracy: true,
            },
        }, {
            UID:      9,
            TestType: testToString,
            Description: `
Float.ToString is able to convert:
1. negative good float32
2. positive accuracy
3. to Scientific format
`,
            Switches: map[string]bool{
                useScientificFormat: true,
                useNegativeFloat32:  true,
                usePositiveAccuracy: true,
            },
...
}

This is a generator function that generates configurations data structure slices. It is obviously went over the limit of 60. Keeping it outside the function means the encouragement of using global variable, which is a bigger concern that keeping a function to a fix 60 length, 40 statements rule.

Some logical functions can easily go beyond 60 like https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/garbagecollector/garbagecollector.go#L164. By setting 60:40, the tool encourages programmers to spin multiple unnecessary small functions consisting of multiple functions OR forcing them not to tidy up their codes (E.g. braces management).

It takes a passionate Go programmer to the extend of altering the configurations so it will generate a lot of unnecessary noises between the new comers and seasoned developers.

Suggestion

Do add heuristic analysis against the function natively OR
Do not fix some brules to 60 lines, 40 statements without thorough research about practical meta-programming and take some actions like: 2.1. Study and revise the brules with thorough discussions and data-driven. 2.2. Setting the linter to only provide information, not warning / action-required.

hollowaykeanho commented 5 years ago

Transferred from https://github.com/golangci/golangci-lint/issues/796

NiseVoid commented 5 years ago

Hello,

If I am not mistaken your problem seems to be with getting warnings for using --enable-all on golangci-lint which also enables more niche linters, like for example funlen? There's a reason not all linters are enabled by default, I would strongly recommend manually enabling linters you want.

While I don't consider using tons of globals as a good thing, this example seems like something you'd normally see in a constant. Is this generated code?

A case can be made for https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/garbagecollector/garbagecollector.go#L164, as it's mostly just comments. This might be an extreme example however.

The idea of this linter is not to ban long functions completely, but instead to warn about signs of code that was hacked together and never cleaned up. Cases where a long function is actually good code are really rare. And you'd //nolint those. Alternatively you can change the limit in your golangci-lint config.

hollowaykeanho commented 5 years ago

While I don't consider using tons of globals as a good thing, this example seems like something you'd normally see in a constant. Is this generated code?

A case can be made for https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/garbagecollector/garbagecollector.go#L164, as it's mostly just comments. This might be an extreme example however.

The idea of this linter is not to ban long functions completely, but instead to warn about signs of code that was hacked together and never cleaned up.

The thing that drove everyone here mad is that funlen's objectives is senseless and with your statements, baseless.

You're measuring LOC of a function without a valid, objective-and-value-driven purpose. If you're worrying about long functions, gocyclo already does the job heuristically by limiting a function complexity to an acceptable limit (thus a reasonable, logical, and purposeful rule). When gocyclo reported a conflict, it automatically triggers developer to break down that complex function into smaller simple functions, thus reducing the LOC in function. funlen however, is only measuring lines and number of statements. It does not care about the function actual purpose at all.

This is the same argument as:

We should have a linter to check a package should have max 1 file by default.
We should have a linter to check a package should set a max of 200 lines of codes per file.

If funlen is guarding against column-length (e.g. 80/120 columns max), I'm fine with it. It's is an acceptable warnings because of IDE/Editor/Viewer compatibility reason.

Guarding against LOC without any purposeful objective is complete brules. It serves only as useful and optional statistical information, not as rule enforcement.

I'm just only pointing you to one of the many examples: one which is used in the industry, and one from my production ecosystem. In fact, I was expecting you to present your statistical data and source codes that conclude 60:40.

If I am not mistaken your problem seems to be with getting warnings for using --enable-all on golangci-lint which also enables more niche linters, like for example funlen? There's a reason not all linters are enabled by default, I would strongly recommend manually enabling linters you want.

You're missing the point: funlen default configuration is the only one that broke --enable-all and worst: without a purpose.

I want all linters (a.k.a. rules checkers) to work properly as rules checkers, not some function LOC scanner tool. No one is saying funlen is a bad tool.

At this point, it serves as a LOC scanner, not a rule checker (linter). The rule it guards makes zero sense.

Even the newly added "niche" whitespace linter is working fine.

Area of Effects

If you want to know how much damages funlen had done:

$ golangci-lint run --color never --enable-all --disable=funlen "$dirpath"

Broke every development machines because of an update on golangci-lint.

I'm being harassed by my apprentices asking where did you get your magic numbers 60 and 40 that is completely unanswerable.

create a configuration file that set funlen instead of using golang-lint off the shelves.

Broke all CI automation testings.

That function you questioned is a table-driven unit-testing generators that creates simulation scenarios. It complies to gochecknoglobals in which it has its reasoning to be backward-compatible, vendor directory compliance, and prevent magical implementation.

commit new codes all over the places with //nolint:funlen

Broke all the source codes.

Are you seriously advising everyone to add //nolint:funlen onto all the source codes where funlen is not even serving a practical objective?

As of the above, sorry about my temper. I hope you understand the amount of damages funlen had brought to the table.

hollowaykeanho commented 5 years ago

All right, since this is an architecture problem, I propose a way to mitigate the problem while minimizing the efforts of changes.

Propose Changes

funlen should not operate without user providing LOC and Statements limits. Its appropriate responses would be:

case LOC is available || STATEMENT is available:
        run funlen and report results
default:
        funlen exits the scanning with true positive.

This should restore --enable-all mode while maintaining funlen inside golangci-lint. When user wants to use funlen to limit their function LOC, they must explicitly provide the LOC and STATEMENT limits. That way, we maintain funlen optional scanning linter nature.

Here are the reasons.

Not Guessable LOC & Statements

LOC & Statements limits are not guessable without understanding the purpose and necessity of the function. It's near impossible (or perhaps impossible) to do so. I gathered some data beforehand for your reference:

48229 -- unoptimized large test configurations generator (http://doi.org/10.13140/RG.2.2.36308.76166, page 8)
...
23746 -- optimized large test configurations generator (http://doi.org/10.13140/RG.2.2.36308.76166, page 8)
...
421 -- ed25519 ScMulAdd (https://github.com/golang/go/blob/master/src/crypto/ed25519/internal/edwards25519/edwards25519.go#L1026)
...
110 -- deep reflect (https://github.com/golang/go/blob/master/src/reflect/deepequal.go#L24)
...
85 -- ed25519 data generator (https://github.com/golang/go/blob/master/src/crypto/ed25519/internal/edwards25519/edwards25519.go#L218)
...
61 -- regex replace all (https://github.com/golang/go/blob/master/src/regexp/regexp.go#L583)
...
47 -- regex (https://github.com/golang/go/blob/master/src/regexp/regexp.go#L169)
...
43 -- bufio longest (https://github.com/golang/go/blob/master/src/bufio/bufio.go#L241)
...
24 - average new comers' smelly codes (https://forum.golangbridge.org/t/whats-issue-with-this-goroutine-code/15663)
...
17 -- average new comers source codes (https://forum.golangbridge.org/t/whats-the-issue-with-this-code/15642/23?u=hollowaykeanho)
...
6 -- average new comers tutorial codes (https://play.golang.org/p/RKezV69HjPe)
...
3 -- minimum interface (https://github.com/golang/go/blob/master/src/bufio/bufio.go#L577)
...
1 -- hello world main (https://play.golang.org/)

Hence, it makes no sense to set a default limit for LOC and Statement respectively. The best way is to query from the user himself/herself before funlen operates.

Funlen is a scanner tool

Including funlen into golangci-lint is acceptable since it acts as a scanner tool. We only need to reconfigure its default behavior and upstream back to golangci-lint for update.

golangci-lint --enable-all has been working fine

All along golangci-lint --enable-all is working fine and their default configuration values are tuned to be industrially acceptable. We should not break it with an optional scanner tool.

Is this a good deal for you?

NiseVoid commented 5 years ago

First of all I'd like to explain that there are many ways to write bad long functions that don't even get a gocyclo score of 1. I frequently see functions that repeat the same 2-3 lines for every field in a struct. Funlen then fails their PR and they have to think about if they can clean up their code.

I understand why your apprentices have complaints, if I randomly have a new linter enabled without discussing this at all, we would also get complaints. Especially when you don't change the code and your CI suddenly fails.

Are you running a release version of golangci-lint? Because I doubt other new linters added after the latest release, like dogsled, godox, and WSL will pass right away. They, like many other linters, are an opinionated vew of what is "good code". Setting --enable-all might work now, but at some point opinions might conflict and fixing one warning could result in another.

We don't have gonoglobals and gonoinit enabled, because we simply don't agree with this opinionated view. If globals and init were evil, why were they added to the language if they were only ever bad features.

If golangci-lint promoted enable-all it would make sense to force people to manually set limits. However this does not seem to be the case, so having a linter you have to enable twice to do anything would seem like it is just broken.

NiseVoid commented 5 years ago

I'd also like to point out the example config in the golangci-lint README uses disable-all and manually enables all wanted linters. And it states the following: inverted configuration with `enable-all` and `disable` is not scalable during updates of golangci-lint

hollowaykeanho commented 5 years ago

First of all I'd like to explain that there are many ways to write bad long functions that don't even get a gocyclo score of 1.

Show me. Show your data, like the way I shown you.

Show everyone and convince people that 60 LOC and 40 statements are the correct numbers and not some magic you conjured up. It's a very sensitive statement across the software industry, not just to Go. If you can't present data, I'm pretty sure you made things up because your opinions.

It is absurd because the complexity is low (e.g. data generation functions) and the structure of the function requires longer than 60 needs to pretty print the codes, does that constitutes bad codes?

Are you saying that FiloSottile did very bad codes on his ScMulAdd function because he hits 421 lines of codes? That's NaCl crypto library FYI.
Are you saying the standard libraries reflect, regex, and bufio package are broken?
Are you saying linux kernels' Designware DMA driver is smelly because it hits more than an opinionated LOC?

I'm not going to argue further with you about long LOC being good or bad codes philosophy. The very fact that I'm supporting funlen to be inside golangci-lint means that I still accept it as a measuring tool but get your default configuration behavior correct.

If I want to put it in layman term:

You just build a ruler to measure the height of the banana tree in order to
determine its health and bearing fruits. Some people will use it to ensure
the tree doesn't go too tall, making it hard to harvest later. You made a
statement:
+--------------------------------------------------+
| "the tree is sick if it grew over **5CM**        |
+--------------------------------------------------+
What is not acceptable is your **5CM** argument.

Are you planning to tell all the farmers around the world to chop down
a perfectly fine 12.5cm banana tree that bears 165 fruits at
the moment because it was "sick" by growing over 5cm?

Also, Where did you get 5cm? Why?
1. Had you tabulated a huge database about banana trees in Brazil,
   U.S, U.K, China, Indonesia, etc. and average it?
2. Had you done No.1 across all species of bananas?
3. Had you considered the soil composition?
4. Had you considered irrigation cycles?

Especially when you don't change the code and your CI suddenly fails.

Are you running a release version of golangci-lint?

If you want people to alter CI codes but it must have a valid and solid reason. Every other optional linters has their valued purpose, and default values configured properly, funlen is not. You want me to list out?

bodyclose - My HTML are confidently safe.
golint - My code is ensured to single standard style of source codes working alongside with gofmt.
stylecheck - same as golint.
gosec - static analysis security checked like Valgrind & BlackDuck in C/C++
interfacer - I got cleaner interface.
unconvert - reduced type conversion.
dupl - clear indicator for me to refactor duplicated codes.
goconst - My constants are effective.
gocyclo - Keeping my function short and simple.
gocognit - same as 9 but cognitively simpler.
gofmt - my code are gofmt-ed.
goimports - my imports statements are organized and tidy.
maligned - my structure is not wasting memory.
depguard - Go linter that checks if package imports are in a list of acceptable packages
misspell - my English is confidently correct.
lll - ensure each codes are not horizontally too long.
unparam - each of my functions' parameters are used effectively and unused are removed.
dogsled - not wasting cpu cycles.
nakedret - return is at where it supposed to be.
prealloc - slice memory are confidently managed.
scopelint - my variables are objectively used.
gocritic - advised on "good" codes, an option.
gochecknoinits - my package does not need init to confuse people.
gochecknoglobals - backward compatible before Go 1.10.
godox - No more leftover works.
funlen - ??? chop a perfectly fine function into multiple functionlets???
whitespace - cleared of whitespace.
wsl - Whitespace Linter - cleared of whitespace.

Can you fill up funlen objective in the list?

I'm already presented you a proper alternation to that default behavior on a win-win deal:

Maintain its inclusion in golangci-lint as a measurement tool.
Only run when input is given because it is very opinionated and default values can't be guessed as I had shown you (1--->40k+ across many people people with different experience and backgrounds).

We don't have gonoglobals and gonoinit enabled, because we simply don't agree with this opinionated view. If globals and init were evil, why were they added to the language if they were only ever bad features.

The fact that even it is opinionated, they don't break things like yours. Also, even they do, they already have a reason to justify that. See the list above. Hence, even I enable it, the attention given to them is worth of its presented values.

I'd also like to point out the example config in the golangci-lint README uses disable-all and manually enables all wanted linters. And it states the following: inverted configuration with enable-all and disable is not scalable during updates of golangci-lint

Should golangci-lint discard and removes --enable-all because of funlen? Do you like that description?

This whole issue is about the false positive behavior by funlen due to misconfigured default behaviors.

If golangci-lint promoted enable-all it would make sense to force people to manually set limits. However this does not seem to be the case, so having a linter you have to enable twice to do anything would seem like it is just broken.

golanci-lint does not promote --enable-all because not everyone is able to understand and manage opinionated tools. Otherwise, they would be very busy answering and aruging philosophical questions like "smelly" codes cuz of LOC.

Besides, keep in mind that golangci-lint users ranges from wide spectrum of customers (from completely new Go developer to very seasoned developer). They had least concerned with the latter.

However, one thing for sure is that golangci-lint team never breaks --enable-all option throughout my history of use. Hence, for seasoned developers like us, we know it works out-of-the-box. When things breaks like the recently whitespace did complained about some source codes, it makes sense: there are wasteful whitespaces.

Just because golangci-lint did an awesome job in offering options to their customers, it does not mean it is an excuse for you not fixing the false-positive problem. They have higher priority things to do.

You are a linter developer, which should had done sufficient background research data to backup your development.

hollowaykeanho commented 5 years ago

It's a very sensitive statement across the software industry, not just to Go.

When I say sensitive, it is scaringly sensitive. Consider the PR attention you need for funlen and compared to my proposal of change. Which is a better deal?

If I'm that opinionated and unreasonable:

I wouldn't take time reading through your codes, analyze the architecture problems for you already.
I would just raise a ticket and focus on lobbying to get rid funlen at all.
I would dead-set argue LOC coding philosophy with you already and season you with Rosemary+Tthyme before slowly grilling you to perfect well done.
I wouldn't support the funlen existence in golangci-lint.

However, it is unreasonable and unethical to remove a ruler just because I do not need it or see any values in it from a great Swiss knife. All I ask, is that the ruler to be included and stop behaving like a sharp long sword and cutting things around. Please, provide a sheath.

Besides, what's the end-goal of the philosophical arguments anyway? It only hurts both of us.

NiseVoid commented 5 years ago

Can you fill up funlen objective in the list?

Yes, funlen is a linter that keeps your codebase from getting long functions that are hard to maintain and impossible to understand.

Show everyone and convince people that 60 LOC and 40 statements are the correct numbers and not some magic you conjured up. It's a very sensitive statement across the software industry, not just to Go. If you can't present data, I'm pretty sure you made things up because your opinions.

There are settings in golangci-lint to configure this limit. It's set to 60 and 40 so it at least warns about long functions by default, rather than having someone turn it on, and them think their code is perfect even though it doesn't check anything. Length of functions is definitely not the only sensitive statement across the software industry, almost all the disabled-by-default linters in golangci-lint are. Even gofmt isn't accepted by everyone. There is a reason why golangci-lint has settings to enable and disable linters.

It is absurd because the complexity is low (e.g. data generation functions) and the structure of the function requires longer than 60 needs to pretty print the codes, does that constitutes bad codes?

There are always exceptions, this is why you can //nolint:[lintername] any linter. Additionally there's the possiblity of ignoring certain files and generated files being ignored (if they are properly marked with // Code generated by [generator]; DO NOT EDIT.)

Are you saying that FiloSottile did very bad codes on his ScMulAdd function because he hits 421 lines of codes? That's NaCl crypto library FYI.

I would not say this function can in any way be considered clean code, it is unreadable and there could easily be hunderds of bugs hiding there.

Are you saying the standard libraries reflect, regex, and bufio package are broken?

There's plenty of short functions in the standard library, going over 60 lines of code is relatively rare outside very complex functions, or functions handling many different types.

Are you saying linux kernels' Designware DMA driver is smelly because it hits more than an opinionated LOC?

The Linux kernel is C code, C is a more verbose language so functions tend to be longer there. If I were to make this linter for C the limit would be higher, as it's very common for functions to be near 100 lines.

Should golangci-lint discard and removes --enable-all because of funlen? Do you like that description?

While this doesn't have anything to do with funlen, I do agree that --enable-all should not be in golangci-lint, there is no good usecase for using every linter without thinking about it.

Just because golangci-lint did an awesome job in offering options to their customers, it does not mean it is an excuse for you not fixing the false-positive problem. They have higher priority things to do.

Which false positives? You write a long function so it warns you about having a long function, nothing false about this. There's already plenty of people using this linter without any problems. On existing code bases you could set the limit a bit higher to reduce the number of warnings. In golangci-lint itself for example this was set to 100 and 50.

As for the arguments for the other linters, they also have similar problems:

dogsled - not wasting cpu cycles.

There are many function in the standard library where it's normal to put multiple underscores, which would make this linter cause significant amounts of warnings on existing code bases.

gochecknoglobals - backward compatible before Go 1.10.

What does this linter have to do with Go 1.10? Globals have been around since before Go 1.0. Many code bases use globals, and there are very good reasons to have globals. This linter is mostly there to prevent the number of globals from spiraling out of control, and for libraries that should be able to use multiple instances.

godox - No more leftover works.

There are many cases where TODOs and FIXMEs are left after a feature is done, for example to note where future new features should be placed, or where improvements can be had.

wsl - Whitespace Linter - cleared of whitespace.

This linter does not just clear whitespace, it also requires newlines on various places. WSL's repo states it is a "very uncientific opinionated vision" of how code should be written. This sounds way more opinionated than two configurable limits.

hollowaykeanho commented 5 years ago

Yes, funlen is a linter that keeps your codebase from getting long functions that are hard to maintain and impossible to understand.

Exactly how many times do I have to tell you show me your data points that 60:40 is the default value and what gives you to right to discriminate one's expertise by the 60:40?

If you want to argue about opinionated point, show data.

The Linux kernel is C code, C is a more verbose language so functions tend to be longer there. If I were to make this linter for C the limit would be higher, as it's very common for functions to be near 100 lines.

Try email directly to DMA mailing list and cc Linus Torvalds with your argument? LOC linters do not matter across languages, moreover Go and C are very close siblings.

Don't worry about Linus grilling, the CoC did tamed him down. It's fine discussing technical stuff with him.

What does this linter have to do with Go 1.10? Globals have been around since before Go 1.0. Many code bases use globals, and there are very good reasons to have globals. This linter is mostly there to prevent the number of globals from spiraling out of control, and for libraries that should be able to use multiple instances.

One high value result from gochecknoglobals is because vendor directory before Go.1.11 (go module) has a limitation with global variables which is the sole reasons this linter has its value. Also, the linter encourages as you said, not to deal with things magically.

Anyway, if you're reluctant to do the changes, I had done it for you: https://github.com/ultraware/funlen/pull/3. It was already cross-checked with your golangci-lint integration codes.

If you still insists, then you left me no choice.

hollowaykeanho commented 5 years ago

Which false positives? You write a long function so it warns you about having a long function, nothing false about this. There's already plenty of people using this linter without any problems. On existing code bases you could set the limit a bit higher to reduce the number of warnings. In golangci-lint itself for example this was set to 100 and 50.

Like I said:

Are you saying that FiloSottile did very bad codes on his ScMulAdd function because he hits 421 lines of codes?
Are you saying the standard libraries reflect, regex, and bufio package are broken?
Are you saying linux kernels' Designware DMA driver is smelly because it hits more than an opinionated LOC?

I'm talking about funlen default behavior NOT questioning its scanning capabilities. Are you on the same page?

hollowaykeanho commented 5 years ago

As for the arguments for the other linters, they also have similar problems:

They are all performing as expected. Only funlen is giving problem and I'm addressing the problem (also produced the necessary fix) for you. Again, are we on the same page?

hollowaykeanho commented 5 years ago

Yes, funlen is a linter that keeps your codebase from getting long functions that are hard to maintain and impossible to understand.

Like I said, how? chop a perfectly okay and presentable function into multiple function-lets? The explanation is different compared to gocyclo.

hollowaykeanho commented 5 years ago

I would not say this function can in any way be considered clean code, it is unreadable and there could easily be hunderds of bugs hiding there.

Or let's make a hands on, how would you process Filosottile's ScMulAdd function under 60:40 if you say something is wrong with his codes? Mind me, this is the default value you enforced on new programmers.

Is it because the code is unclean or you does not know how the math works behinds edwards25519?

jirfag commented 5 years ago

I would like to add about golangci-lint: I think --enable-all should be removed, and maybe I will do in v2.0.0. The reason is this case: unexpected CI failures after linter update. I see it as invalid usage of golangci-lint and I've written about it many times in README but it doesn't help. Maybe we can deprecate this option even in v1 major versions.

@hollowaykeanho why don't you just disable funlen in golangci-lint?

hollowaykeanho commented 5 years ago

@hollowaykeanho why don't you just disable funlen in golangci-lint?

Because --enable-all has no problems at all. In fact, after upgrading to 1.19.1, I'm very happy with the additional optional linters (especially wsl and whitespace because of their outstanding performances). The warnings, detection, and everything are all working excellently.

What is not fine is funlen author failed to present solid data to support his/her 60:40 points and keep blaming golangci-lint for not handling the user manual well. This is absolutely absurd as golangci-lint did a great job compared to the past, gometalinter. When challenged, all he/she did was pointlessly and mindlessly arguing and forcing people to "write a configuration file" for an already expected default behavior.

The worst part is he/she even failed to present a "proposed action" when the linter hits. That's why I ask him/her to demonstrate how he/she is going to chop a famous cryptography engineer's one small little data generation function from ED25519 where he/she claimed it is smelly by his/her 60:40 LOCs standards.

If you present it to your boss about you finding, what do you want to say? -- "linter author want it his/her way, no data presented, is all he said she said so we assigned our engineers to alter our infrastructure for no solid reason."

As far as I diagoned, --enable-all and --disable-all both are not guilty. In this case, the author is.

hollowaykeanho commented 5 years ago

The reason is this case: unexpected CI failures after linter update. I see it as invalid usage of golangci-lint and I've written about it many times in README but it doesn't help.

Yes, it is clear. The thing is I'm expecting breakdown after the CI and linter upgrades. It's my responsibility to handle any new errors. What is not acceptable is linter has non-explainable standards that turns up magically. The issue is the latter.

hollowaykeanho commented 5 years ago

I think --enable-all should be removed, and maybe I will do in v2.0.0.

If you want a post mortem for improvements, it should be vetting linter's default values because those can become coding standards for Go after imposing them. This is a 1 time issue where linter failed to explains the standards it imposes.

The user manual are fine. Stop altering here and there. We know what to do with locking and prepare for CI. I do not see other linters are giving issues.

NiseVoid commented 5 years ago

I would like to add about golangci-lint: I think --enable-all should be removed, and maybe I will do in v2.0.0. The reason is this case: unexpected CI failures after linter update. I see it as invalid usage of golangci-lint and I've written about it many times in README but it doesn't help. Maybe we can deprecate this option even in v1 major versions.

@jirfag I think deprecating it by showing a warning at first, and later exiting with a clear explanation of why people shouldn't use it and what they should do instead would be an acceptable way to do this. As the number of linters grows, the problems will only get worse. At some point linters might even conflict with each other.

Or let's make a hands on, how would you process Filosottile's ScMulAdd function under 60:40 if you say something is wrong with his codes? Mind me, this is the default value you enforced on new programmers.

There's nothing I enforce on any programmer, golangci-lint's default config clearly states to not --enable-all. It also shows you how to change the limits. I think it's safe to assume people will enable only linters they agree with.

As for the ScMulAdd case, I see a few repeating patterns so there's ways to shorten this code and make it more clear what is happening. The reason this hasn't been done is that there is no way to enforce inlining on functions and it might require arrays. Those wouldn't be a problem for 99% of the use cases (where readable code is more important than a small amount of performance), but in encryption this could be a problem.

Since the maintainer of golangci-lint stated --enable-all is misuse and this would only be a problem with that flag. I don't see any issue with the way funlen is currently implemented.