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.