chore: Lint tests by faddat · Pull Request #5709 · caddyserver/caddy

faddat · 2023-08-06T06:30:43Z

THis PR lints the tests in caddy, too.

adds thelper linter (gives additional context in your editor when tests fail)
adds revive linter
lints the entire codebase
adds nolintlint so that nolint statements don't hang around where they're not needed

CLAassistant · 2023-08-06T06:30:48Z

All committers have signed the CLA.

faddat

reviews sometimes can be quite not ergonomic

(this comment was required to complete a review)

mholt · 2023-08-14T17:03:50Z

Hmmm. I disagree with the linter in some places here. I think a field name like publicKey is perfect because it's the final result of processing the PublicKeys field after provisioning from config.

faddat · 2023-08-14T17:21:07Z

@mholt -- indeed, I disagreed with it too :).

Please lmk if I made any changes based on its disagreeable choices.

I basically finished this one out by disabling a few things:

confusing-naming
unhandled-error
import-shadowing

I'f you'd like any of them on, please let me know that too :)

Ah, this one still has a few traces of unused-parameter in it, I'll get those out too.

mholt

What's the problem with turning unused parameters into parameters named _? I do that sometimes, maybe it's good to be consistent...

I guess we do lose the self-documenting code a bit though.

faddat · 2023-08-14T17:31:03Z

@mholt -- those _'s were kinda the result of a misconfiguration.

I think that their clarity (eg: we don't do anything with this) is less-good than the self-documenting code. I did this on ibc-go a while ago and ended up having to seek out names a bunch.

Overall, I wanted to let the maintainers here know, the idea behind this set of PR's was to make writing code easier by transferring as much of the formatting work to a machine as possible. In the long run I'd recommend progressively tightening the linters, because you can catch small/easy issues (that can be maddening) that way, and also ensure that the code is really rigorously formatted.

The reason I came to do this now, was the tweet about the feature freeze -- that's pretty much a perfect time to do this kind of work, because when linting we don't want to change what the code does at all (except maybe adding some tests, the linters are pretty good at finding places where we could add additional tests).

Ah, and about linting the tests -- so that's not default behavior for golangci-lint but it probably should be. Once you're linting all of the code including the tests, you can get good consistency on the whole codebase, really fast.

You can think of the linter configuration, as the thing that handles all of the formatting of the code, and in the future, you can use it to make sometimes surprisingly granular changes even to small stuff.

mholt · 2023-09-01T16:43:26Z

 	if IsUnixNetwork(network) {
 		host = a
-		return
+		return network, host, port, nil


This seems like a mistake -- the returns are named.

ahhh, so it isn't a mistake actually, it's designed to make the code clearer. If you'd like me to change it back the way it was, just let me know.

mholt · 2023-09-01T17:11:03Z


 // Provision sets up the header operations.
-func (ops *HeaderOps) Provision(_ caddy.Context) error {
+func (ops *HeaderOps) Provision(ctx caddy.Context) error {


This variable isn't used, so I'm not sure why it needs a name. Naming it falsely implies that we're using it, whereas _ makes it clear we're satisfying an interface IMO. I know we talked about it before but it still seems misleading to me. Especially for an argument where the type makes things obvious; if this was a string or int, with a very vague function name, maybe a different matter.

OK, I can change that setting, and that'd get applied universally.

Universally for context args, or for every arg?

Because I think it could make sense for some args.

If it can't be configured in that way, I don't think that linter is helpful for us.

mholt · 2023-09-01T17:11:21Z

 	}

-	var false bool
+	var falseBool bool


Why this change?

it was shadowing the name "false"

mholt · 2023-09-01T17:11:36Z

 	err = writer.writeBeginRequest(uint16(Responder), 0)
 	if err != nil {
-		return
+		return nil, err


More named returns

AnomalRoil

Here are a few comments.

I think I agree with @mholt that keeping the _ instead of using named but unused parameters is clearer.

But I also think that if you're replacing named return with explicit returns, you might as well drop the named returns most of the time.
(Despite @mholt clearly loving these named returns, I think it doesn't help with readability of long functions, so it's probably better to have explicit return values.)

AnomalRoil · 2023-11-30T22:42:03Z

 			full = CustomVersion
 			simple = CustomVersion
-			return
+			return full, simple


This is permuting both strings, no?

Suggested change

return full, simple

return simple, full

Not that it matters here, but better be consistent.

AnomalRoil · 2023-11-30T22:42:51Z

 		full = "unknown"
 		simple = "unknown"
-		return
+		return full, simple


This is permuting both strings, no?

Suggested change

return full, simple

return simple, full

Not that it matters here, but better be consistent.

AnomalRoil · 2023-11-30T22:43:44Z

 		// Set the ETag as a trailer header.
 		// The alternative is to write the config to a buffer, and
 		// then hash that.
+


Why the extra line here?
The comment is related to the next line.

Suggested change

AnomalRoil · 2023-11-30T22:47:38Z

 linters-settings:
+  revive:
+    enable-all-rules: true
+    # Do NOT whine about the following, full explanation found in:


Not convinced anybody "whines".

Suggested change

# Do NOT whine about the following, full explanation found in:

# Had to disable these out of the 77 rules. Full explanation of all rules found in:

Oh I think they're actually referring to the linter complaining when we don't really care about these things or they aren't relevant in our project. :) Not a person. But yeah, maybe the proposed change here is better.

AnomalRoil · 2023-11-30T22:49:01Z

+      - name: unhandled-error
+        disabled: true
+        # Arguments added below do not trigger the rule.
+        #           arguments =["os\.(Create|WriteFile|Chmod)", "fmt\.Print", "myFunction", "net\..*", "bytes\.Buffer\.Write"]


Suggested change

# arguments =["os\.(Create|WriteFile|Chmod)", "fmt\.Print", "myFunction", "net\..*", "bytes\.Buffer\.Write"]

AnomalRoil · 2023-11-30T23:28:37Z

 }

-func (e StaticError) ServeHTTP(w http.ResponseWriter, r *http.Request, _ Handler) error {
+func (e StaticError) ServeHTTP(w http.ResponseWriter, r *http.Request, h Handler) error {


this feels like it hurts readability. I agree with what @mholt was saying in an above comment, better keep the _ for these function whose goal is to fulfill an interface.

AnomalRoil · 2023-11-30T23:29:20Z

 	resp := w.Result()
+	resp.Body.Close()
 	respBody, _ := io.ReadAll(resp.Body)


Suggested change

resp := w.Result()

resp.Body.Close()

respBody, _ := io.ReadAll(resp.Body)

resp := w.Result()

respBody, _ := io.ReadAll(resp.Body)

resp.Body.Close()

You want to read the Body before closing it ;)

AnomalRoil · 2023-11-30T23:33:27Z


 // canHazCertificate returns true if Tailscale reports it can get a certificate for the given ClientHello.
-func (ts Tailscale) canHazCertificate(ctx context.Context, hello *tls.ClientHelloInfo) (bool, error) {
+func (Tailscale) canHazCertificate(ctx context.Context, hello *tls.ClientHelloInfo) (bool, error) {


😆 Very tempted to git blame who came up with that naming, fitting of Tailscale being the cool kids tho!

That might have been me, being commissioned by Tailscale (to dev the feature, not to name it that, lol). 😶

AnomalRoil · 2023-11-30T23:36:50Z


 // Provision sets up the module
-func (fw *FileWriter) Provision(ctx caddy.Context) error {
+func (fw *FileWriter) Provision(caddy.Context) error {


Suggested change

func (fw *FileWriter) Provision(caddy.Context) error {

func (fw *FileWriter) Provision(_ caddy.Context) error {

AnomalRoil · 2023-11-30T23:42:35Z

 			// logger socket still offline; instead of discarding the log, dump it to stderr
 			os.Stderr.Write(b)
-			return
+			return 0, err2


This changes the logic, but maybe in a good way.

Might be worth considering returning errors.Join of both err and err2 or maybe a new error wrapping both, tho.

francislavoie · 2023-12-01T00:32:11Z

Thanks for the review comments @AnomalRoil, appreciated!

This PR will need rebasing, it's fallen a bit behind.

mholt · 2023-12-01T01:06:00Z

Yes, thank you from me too, @AnomalRoil !

But I also think that if you're replacing named return with explicit returns, you might as well drop the named returns most of the time.
(Despite @mholt clearly loving these named returns, I think it doesn't help with readability of long functions, so it's probably better to have explicit return values.)

If I read you right, I think I agree. Named returns can be very elegant sometimes and even help improve correctness / reduce error-proneness, but if most/all of the return statements end up using variable names, the names should be removed from the return signature.

@faddat Any interest in finishing this up? Apologies if it's overwhelming. That's just what happens sometimes with big changes 😅 We appreciate it!

mholt · 2024-03-05T19:11:52Z

Closing due to inactivity -- however, we'd be open to finishing this when you are @faddat (or anyone else who would like to pick this up)!

Maybe it would be easier to break it down into smaller changes if possible.

faddat added 3 commits August 6, 2023 14:20

use gofmput to format code

ec654b2

use gci to format imports

00c5444

lint tests, too

b6a44e8

faddat added 12 commits August 6, 2023 14:41

naked return checks

74cc70f

continue linting tests

6d529e3

reconfigure gci

84504d3

linter autofixes

4b47929

rearrange imports a little

c4a08bd

all changes other than unused-parameter

8cd1134

use revive linter

c416fb8

enable thelper

db5de9b

Merge branch 'revive' into lint-tests

4b370b9

autofixes (golangci-lint run ./... --fix)

ba34aa4

further clean tests

968dbc1

continue linting tests

2358852

faddat mentioned this pull request Aug 6, 2023

Linting #5706

Closed

faddat commented Aug 14, 2023

View reviewed changes

faddat force-pushed the lint-tests branch from e5e3d2d to 2358852 Compare August 14, 2023 16:28

faddat added 3 commits August 15, 2023 00:34

Merge remote-tracking branch 'origin/master' into lint-tests

3105a69

update test lints

273455c

unused-recievers

2bf86fa

lint tests

c41a8cd

faddat added 2 commits August 15, 2023 01:22

remove a _

6790811

reverse some unused-reciever changes

e7d92a1

mholt reviewed Aug 14, 2023

View reviewed changes

Comment thread .github/dependabot.yml Outdated

Comment thread modules/caddyhttp/push/handler.go Outdated

_ -> ctx

3fbbfa3

faddat mentioned this pull request Aug 14, 2023

chore: use revive linter #5712

Closed

revert changes to dependabot

b71f517

faddat added 4 commits August 15, 2023 01:35

restore an "unused parameter"

4b22b11

add nolintlint

54fcb8e

nolintlint

eb5741f

return the error cleanly (handler.go)

8944632

faddat marked this pull request as ready for review August 14, 2023 17:51

faddat added 7 commits August 15, 2023 02:23

revert addition of blanks

813b684

revert addition of blanks

cd0e19e

revert addition of blanks

b113c3a

revert addition of blanks

756ae11

revert addition of blanks

14b54e9

revert additon of blanks

24ab9f4

Merge branch 'master' into lint-tests

c386c0a

mholt reviewed Sep 1, 2023

View reviewed changes

francislavoie changed the title ~~lint tests~~ chore: Lint tests Sep 8, 2023

Merge remote-tracking branch 'origin/master' into lint-tests

30e99a2

AnomalRoil reviewed Nov 30, 2023

View reviewed changes

mholt closed this Mar 5, 2024

mholt added the needs info 📭 Requires more information label Mar 5, 2024

	# Do NOT whine about the following, full explanation found in:
	# Had to disable these out of the 77 rules. Full explanation of all rules found in:

	func (fw *FileWriter) Provision(caddy.Context) error {
	func (fw *FileWriter) Provision(_ caddy.Context) error {

Uh oh!

Conversation

faddat commented Aug 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented Aug 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

faddat left a comment • 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!

mholt commented Aug 14, 2023

Uh oh!

faddat commented Aug 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mholt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

faddat commented Aug 14, 2023 • 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!

mholt Sep 1, 2023 • 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!

AnomalRoil left a comment

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!

francislavoie commented Dec 1, 2023

Uh oh!

mholt commented Dec 1, 2023

Uh oh!

mholt commented Mar 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

faddat commented Aug 6, 2023 •

edited

Loading

CLAassistant commented Aug 6, 2023 •

edited

Loading

faddat left a comment •

edited

Loading

faddat commented Aug 14, 2023 •

edited

Loading

faddat commented Aug 14, 2023 •

edited

Loading

mholt Sep 1, 2023 •

edited

Loading