🚀 Feature: Add and apply more stricter golangci-lint linting rules

[leonklingele]

Description

In order to fix and avoid upcoming bugs, apply more stricter linting rules while still being easy to maintain.

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

Checklist:

• For new functionalities I follow the inspiration of the express js framework and built them similar in usage
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation - https://github.com/gofiber/docs for https://docs.gofiber.io/
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• If new dependencies exist, I have checked that they are really necessary and agreed with the maintainers/community (we want to have as few dependencies as possible)
• I tried to make my code as fast as possible with as few allocations as possible
• For new code I have written benchmarks so that they can be analyzed and improved

[leonklingele]

This is quite a big diff, but should be rather quick to review.

[ReneWerner87]

is it still a draft or already final

I will then look at everything

[leonklingele]

I still need to fix the linting issues in all subpackages. Hope to complete that until next week.

[leonklingele]

This is now ready for review, @ReneWerner87
Tests are flaky but they were before.

[efectn]

LGTM. We should resolve some TODOs in v3 later.

[leonklingele]

@ReneWerner87 I'll rebase this once approved.

[leonklingele]

Any news here? 😊 I'd like to get this merged asap. Resolving the conflicts will become more and more difficult with time.

[ReneWerner87]

Sorry will check it in a few hours

[ReneWerner87]

@leonklingele can you resolve the conflicts one more time -> before i merge it

We may have to readjust some of the rules again(later).

some adjustments like the initialization of the return variable although covered by golang i wouldn't make it mandatory and leave the developers the freedom to do so
f.e.

also don't know which lint rule this is, but maybe we can disable it

but most of the stuff is great

[leonklingele]

also don't know which lint rule this is, but maybe we can disable it

@ReneWerner87 that's the nonamedreturns linter. Also, I renamed exist -> exists, but that wasn't reported as an issue.

I'll rebase this now.

[leonklingele]

@ReneWerner87 any idea why the Windows tests are failing now?

[ReneWerner87]

i don´t see any big mistake

only this is different

here we handle the error instead of ignoring it
but its just logging

[ReneWerner87]

@leonklingele
have you found the error yet ? have my windows computer ready today and will try to find the problem so we can merge soon

will gradually undo your changes locally and see if I can locate the point of failure

[ReneWerner87]

have tried to fix the errors, but that is currently not possible because I have limited time

in the pull request more has been implemented than is initially apparent

• the linting was configured and the rules were applied
• some errors were wrapped for better output
• parallelism of tests was activated

exactly the last point is the problem, because not all tests or the whole app is currently not yet built to run in parallel with multiple instances

@leonklingele if i were you i would disable this parallelism in some places and consider this separately from the pull request into a new one as it takes more time and prevents the big swing of changes from landing in the master

[leonklingele]

• parallelism of tests was activated

No, this was already done previously as part of #2299. None of the tests are made parallel with this change.

I have rebased to latest master to resolve merge conflicts and also cleaned up some of the new commits (yours + mine).
All tests are passing now! 🥳🥳🥳 @ReneWerner87 I guess this can be merged, finally? 😃

[gaby]

@ReneWerner87 @efectn @leonklingele

I feel like this PR needs to be reverted, and applied by section. So far, I have found some slight issues:

• The monitor middleware no longer displays metrics for CPU, Memory, and Connections.
• The monitor middleware API now returns 0 for all fields.
• Changes were made to 3rd party libraries code in the internal folder.
• The Fiber client was changed to trigger panics.
• The ctx.go now triggers a panic if !ok when using a Pool.
• Error returns were replaced with fmt.Errorf in a lot of places, specially in app.Test() this will affect how people write unit-tests for their servers.
• Changes to casing/variable names are no replicated in the documentation, causing confusion.
• The codebase is now plague with nolint directives for things that are valid in Go.

From my point of view, the Monitor middleware tests are all passing just fine but the middleware does not work at all. What else could be broken that we haven't noticed yet?

[leonklingele]

Thanks for the feedback @gaby! To address your comments:

The monitor middleware no longer displays metrics for CPU, Memory, and Connections.

The monitor middleware API now returns 0 for all fields.

I have checked the code and it is a bug of the new error handling:

 func updateStatistics(p *process.Process) {
-    pidCpu, _ := p.CPUPercent()
-    monitPidCpu.Store(pidCpu / 10)
+    pidCPU, err := p.CPUPercent()
+    if err != nil {
+        monitPIDCPU.Store(pidCPU / 10) //nolint:gomnd // TODO: Explain why we divide by 10 here
+    }

The code should check for err == nil instead of err != nil as currently done.
Tests would have caught this and should be added.

Changes were made to 3rd party libraries code in the internal folder.

Obviously, this should not have been done. The internal folder is also explicitly excluded from linting for this reason.
Which of the modified files inside internal belong to 3rd party libs? And what's the reason we don't use them as a module? (I've seen them getting removed in v3, but why don't we do it for v2 as well?)

The Fiber client was changed to trigger panics.

@gaby are you referring to the changes which now validate the second (bool) return value of type-assertions?

@@ -877,7 +882,11 @@ func AcquireClient() *Client {
     if v == nil {
         return &Client{}
     }
-    return v.(*Client)
+    c, ok := v.(*Client)
+    if !ok {
+        panic(fmt.Errorf("failed to type-assert to *Client"))
+    }
+    return c
 }

This was panic-ing before (implicitly), so it's not a breaking change.

The ctx.go now triggers a panic if !ok when using a Pool.

See above. This was panic-ing before, although now done explicitly instead of implicitly.

Error returns were replaced with fmt.Errorf in a lot of places, specially in app.Test() this will affect how people write unit-tests for their servers.

Those errors are now returned in a wrapped form with some additional context. Indeed it is a breaking change for people who do not use errors correctly (e.g. by comparing errors against a string).
For reference, we also do that at least once in the codebase:

@@ -1379,7 +1381,7 @@ func Test_Test_DumpError(t *testing.T) {
     resp, err := app.Test(httptest.NewRequest(MethodGet, "/", errorReader(0)))
     utils.AssertEqual(t, true, resp == nil)
-    utils.AssertEqual(t, "errorReader", err.Error())
+    utils.AssertEqual(t, "failed to dump request: errorReader", err.Error())
 }

Instead of comparing the error-string returned by err.Error() here, one should use errors.Is(err, errorReader). Of course, this requires the user to pre-define the errorReader error somewhere, so it can be checked against.

Changes to casing/variable names are no replicated in the documentation, causing confusion.

@gaby which variables are you referring to? No changes to exported fields, funcs, types, etc. should have been made. Renaming unexported data should be fine (and IMO also not be part of the documentation).

Edit: The exported ChartJsURL config field of the monitor middleware was renamed. This should be reverted and a //nolint:stylecheck // TODO: Rename to "ChartJSURL" in v3 be added.

The codebase is now plague with nolint directives for things that are valid in Go.

Valid Go is not necessarily good Go. I quickly skimmed over all //nolint: directives in the codebase, and although quite a few, using them seems reasonable to me (and some have a TODO inside to get rid of them).
@gaby are there any of these directives which do not seem valid to you or is it simply the amount of them which bugs you?

[gaby]

pidCPU, err := p.CPUPercent()

1. The change where now the error codes are being check on stuff that wasn't before completely changes the behavior of Fiber. In this case it caused a bug that bypasses the unit-test. The same could be happening in all the other middlewares/places were the same style of change was applied.
2. I'm not a fan of throwing panics for the user, this will cause fiber to completely shutdown unless the user is using the recover middleware. It should be on the user to check the variable before using it.
3. From my experience if the return error messages are changed, I will have to go back and update the code for several clients services. This is a breaking change for sure.
4. Fixing ChartJsURL by adding another nolint makes my last point even more valid. What's the point of having such a strict linter if the code is going to be plague by nolint directives?

[ReneWerner87]

#2317 (comment)