Suggestion to introduce golangci-lint

[gold-kou]

Hello.
Now, only used vet and fmt as linters.
It's not a problem but we can use more linters by golangci-lint.
It might be better to introduce more linters as some maintainers now.

This is a sample result of local.
(default setting)

$ golangci-lint run ./...
expectations_go19_test.go:39:11: Error return value of `tx.Commit` is not checked (errcheck)
	tx.Commit()
	         ^
expectations_test.go:57:9: Error return value of `db.Exec` is not checked (errcheck)
	db.Exec(query)
	       ^
expectations_test.go:58:12: Error return value of `db.Prepare` is not checked (errcheck)
	db.Prepare(query)
	          ^
query_test.go:33:10: Error return value of `rs.Scan` is not checked (errcheck)
		rs.Scan(&id, &title)
		       ^
rows_test.go:31:10: Error return value of `rs.Scan` is not checked (errcheck)
		rs.Scan(&id, &title)
		       ^
rows_test.go:61:10: Error return value of `rs.Scan` is not checked (errcheck)
		rs.Scan(&id, &title)
		       ^
rows_test.go:150:10: Error return value of `db.Query` is not checked (errcheck)
	db.Query("SELECT")
	        ^
rows_test.go:472:10: Error return value of `recover` is not checked (errcheck)
		recover()
		       ^
rows_test.go:475:10: Error return value of `db.Query` is not checked (errcheck)
	db.Query("SELECT ID FROM TABLE", 101)
	        ^
sqlmock_go18_test.go:469:16: Error return value of `db.ExecContext` is not checked (errcheck)
	db.ExecContext(context.Background(), "INSERT INTO articles (title) VALUES (?)", "hello")
	              ^
sqlmock_go18_test.go:560:9: Error return value of `db.Ping` is not checked (errcheck)
	db.Ping()
	       ^
sqlmock_test.go:874:10: Error return value of `db.Begin` is not checked (errcheck)
	db.Begin()
	        ^
sqlmock_test.go:977:11: Error return value of `tx.Commit` is not checked (errcheck)
	tx.Commit()
	         ^
sqlmock_test.go:1011:15: Error return value of `rows.Scan` is not checked (errcheck)
		if rows.Scan(&id, &status); id != 101 || status != "Hello" {
		            ^
sqlmock_test.go:1016:11: Error return value of `tx.Commit` is not checked (errcheck)
	tx.Commit()
	         ^
sqlmock_test.go:1047:10: Error return value of `db.Begin` is not checked (errcheck)
	db.Begin()
	        ^
sqlmock_test.go:1136:9: Error return value of `db.Exec` is not checked (errcheck)
	db.Exec("INSERT INTO articles (title) VALUES (?)", "hello")
	       ^
column_test.go:33:2: ineffectual assignment to `nullable` (ineffassign)
	nullable, ok = column3.IsNullable()
	^
column_test.go:42:2: ineffectual assignment to `length` (ineffassign)
	length, ok = column2.Length()
	^
column_test.go:46:2: ineffectual assignment to `length` (ineffassign)
	length, ok = column3.Length()
	^
expectations.go:337:2: `expectSQL` is unused (structcheck)
	expectSQL string
	^
expectations.go:264:2: `statement` is unused (structcheck)
	statement    driver.Stmt
	^
expectations.go:24:2: `err` is unused (structcheck)
	err       error
	^
expectations_test.go:22:15: S1028: should use fmt.Errorf(...) instead of errors.New(fmt.Sprintf(...)) (gosimple)
		return nil, errors.New(fmt.Sprintf("cannot convert %T with value %v", v, v))
		            ^
expectations_test.go:14:9: S1034: assigning the result of this type assertion to a variable (switch v := v.(type)) could eliminate type assertions in switch cases (gosimple)
	switch v.(type) {
	       ^
sqlmock_go18_test.go:438:2: S1021: should merge variable declaration with assignment on next line (gosimple)
	var delay time.Duration
	^
sqlmock_go18_test.go:534:2: S1021: should merge variable declaration with assignment on next line (gosimple)
	var delay time.Duration
	^
sqlmock_test.go:1105:2: S1021: should merge variable declaration with assignment on next line (gosimple)
	var delay time.Duration
	^
query.go:9:10: S1007: should use raw string (`...`) with regexp.MustCompile to avoid having to escape twice (gosimple)
var re = regexp.MustCompile("\\s+")
         ^
sqlmock_test.go:1247:17: SA1019: got.Exec is deprecated: Drivers should implement StmtExecContext instead (or additionally).  (staticcheck)
	result, err := got.Exec([]driver.Value{"test"})
	               ^
sqlmock_test.go:1256:15: SA1019: got.Query is deprecated: Drivers should implement StmtQueryContext instead (or additionally).  (staticcheck)
	rows, err := got.Query([]driver.Value{"test"})
	             ^
sqlmock_test.go:244:2: SA4006: this value of `tx` is never used (staticcheck)
	tx, err = db.Begin()
	^
sqlmock_test.go:1152:2: SA5001: should check returned error before deferring db.Close() (staticcheck)
	defer db.Close()
	^
driver_test.go:10:6: type `void` is unused (unused)

Could you tell me how do you feel about this?
I feel it deserves to try in this repo.

If we keep using Travis, this article might be helpful.
https://medium.com/@classik19881/ci-continuous-integration-with-travis-ci-for-golang-project-532d1d1fc7b6

Or we can use other CI tools such as CircleCI or Actions.

Thank you.

[rmulley]

I think this would be a nice addition to the CI pipeline. golangci is a stable enough and standard enough tool that I would feel comfortable pulling it in. It should help us find additional bugs as well as cleanup and simplify the codebase.

I envision the steps for implementing golangci-lint being:

• Replace gofmt and go vet in the CI pipeline with golangci-lint.
• Ensure all other linters that golangci-lint provides are disabled.
• Generate a green build and merge.
• Gradually enable other linters via golangci-lint, cleaning up the code at the same time to ensure a passing build.

Does that sound reasonable?

[gold-kou]

@rmulley
Thank you for your reaction. 😄

I agree step by step introducing for safe.

However, I couldn't understand that well.
In your idea, PR should be divided?

[rmulley]

@gold-kou I'm saying we should strive to keep builds passing, and avoid very large Pull Requests. To do that we should incrementally bring in golangci-lint and add additional linters. It should not all be done in one step or one PR in my opinion.