RFC: Update go dependencies, replace logrus with slog and fix some linter issues #508
Conversation
There was a problem hiding this comment.
Hey @majst01,
First, thanks for the PR! I still have to review the changes that are removing code and replacing Logrus with slog, but here is a quick initial review with a few questions.
| @@ -56,7 +56,7 @@ func (suite *DatumBinarySuite) TearDownSuite() { | |||
| } | |||
|
|
|||
| func (suite *DatumBinarySuite) TestCases() { | |||
| suite.T().Log("Running DatumBinarySuite: Tests of converstion to and from the RQL binary type") | |||
| suite.T().Log("Running DatumBinarySuite: Tests of conversation to and from the RQL binary type") | |||
There was a problem hiding this comment.
This should be conversion, not conversation.
| suite.T().Log("Running DatumBinarySuite: Tests of conversation to and from the RQL binary type") | |
| suite.T().Log("Running DatumBinarySuite: Tests of conversion to and from the RQL binary type") |
| @@ -56,7 +56,7 @@ func (suite *DatumStringSuite) TearDownSuite() { | |||
| } | |||
|
|
|||
| func (suite *DatumStringSuite) TestCases() { | |||
| suite.T().Log("Running DatumStringSuite: Tests of converstion to and from the RQL string type") | |||
| suite.T().Log("Running DatumStringSuite: Tests of conversation to and from the RQL string type") | |||
There was a problem hiding this comment.
This should be conversion, not conversation.
| suite.T().Log("Running DatumStringSuite: Tests of conversation to and from the RQL string type") | |
| suite.T().Log("Running DatumStringSuite: Tests of conversion to and from the RQL string type") |
| @@ -24,8 +24,6 @@ func init() { | |||
| flag.Parse() | |||
| } | |||
|
|
|||
| r.SetVerbose(true) | |||
There was a problem hiding this comment.
Why do we need to remove setting verbose mode?
| @@ -18,8 +16,6 @@ func init() { | |||
| flag.Parse() | |||
| } | |||
|
|
|||
| r.SetVerbose(true) | |||
There was a problem hiding this comment.
Why do we need to remove setting verbose mode?
| func testSetup(m *testing.M) { | ||
| var err error | ||
| session, err = r.Connect(r.ConnectOpts{ | ||
| Address: url, | ||
| }) | ||
| if err != nil { | ||
| r.Log.Fatalln(err.Error()) | ||
| panic(err) |
There was a problem hiding this comment.
Not sure about the rest of the file, but would be good to follow the same pattern as previously, like something similar to:
| panic(err) | |
| panic(fmt.Errorf("Error connecting to instance: %w", err)) |
| @@ -84,9 +80,6 @@ func testBenchmarkTeardown() { | |||
| } | |||
|
|
|||
| func TestMain(m *testing.M) { | |||
| // seed randomness for use with tests | |||
| rand.Seed(time.Now().UTC().UnixNano()) | |||
|
|
|||
There was a problem hiding this comment.
Why removing the random seed?
There was a problem hiding this comment.
| @@ -82,7 +83,7 @@ func (n *Node) SetMaxOpenConns(openConns int) { | |||
| // processed by the server. Note that this guarantee only applies to queries | |||
| // run on the given connection | |||
| func (n *Node) NoReplyWait() error { | |||
| return n.pool.Exec(nil, Query{ // nil = connection opts' timeout | |||
| return n.pool.Exec(context.TODO(), Query{ // nil = connection opts' timeout | |||
There was a problem hiding this comment.
As context.TODO() is a non-nil context, this line actually changes the behavior that described in the comment in the same line.
There was a problem hiding this comment.
passing a nil context is discouraged by static check, added comparison to context.TODO() in Query to behave the same.
| initialCap := opts.InitialCap | ||
| if initialCap <= 0 { | ||
| // Fallback to MaxIdle if InitialCap is zero, this should be removed | ||
| // when MaxIdle is removed | ||
| initialCap = opts.MaxIdle | ||
| } |
There was a problem hiding this comment.
Why would we remove initialCap?
| @@ -83,7 +82,7 @@ func typeFields(t reflect.Type) []field { | |||
| next := []field{{typ: t}} | |||
|
|
|||
| // Count of queued names for current level and the next. | |||
| count := map[reflect.Type]int{} | |||
| var count map[reflect.Type]int | |||
There was a problem hiding this comment.
I would leave it as it is, but if we want to change it anyway, it would be better to have the following.
count := make(map[reflect.Type]int, 0)The var count map[reflect.Type]int would make an uninitialized map, so assigning any value to it right away will end up in a panic. Also, if we change this line, the next line should be changed too.
There was a problem hiding this comment.
count is assigned in line 96, if already assigned in 86, its never used.
| @@ -84,9 +80,6 @@ func testBenchmarkTeardown() { | |||
| } | |||
|
|
|||
| func TestMain(m *testing.M) { | |||
| // seed randomness for use with tests | |||
| rand.Seed(time.Now().UTC().UnixNano()) | |||
|
|
|||
There was a problem hiding this comment.
| @@ -82,7 +83,7 @@ func (n *Node) SetMaxOpenConns(openConns int) { | |||
| // processed by the server. Note that this guarantee only applies to queries | |||
| // run on the given connection | |||
| func (n *Node) NoReplyWait() error { | |||
| return n.pool.Exec(nil, Query{ // nil = connection opts' timeout | |||
| return n.pool.Exec(context.TODO(), Query{ // nil = connection opts' timeout | |||
There was a problem hiding this comment.
passing a nil context is discouraged by static check, added comparison to context.TODO() in Query to behave the same.
| initialCap := opts.InitialCap | ||
| if initialCap <= 0 { | ||
| // Fallback to MaxIdle if InitialCap is zero, this should be removed | ||
| // when MaxIdle is removed | ||
| initialCap = opts.MaxIdle | ||
| } |
Reason for the change
With recent go versions slog comes as a decent logging implementation
sloguse this instead of logrus. Also some of the most obvious linter issues found by golangci-lint have been fixed.Also most typos found by https://github.com/crate-ci/typos have been fixed.
This is meant as a starting point for further renewal of this excellent library.