-
-
Notifications
You must be signed in to change notification settings - Fork 182
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RFC: Update go dependencies, replace logrus with slog and fix some linter issues #508
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -24,8 +24,6 @@ func init() { | |||
flag.Parse() | |||
} | |||
|
|||
r.SetVerbose(true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to remove setting verbose mode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-added
@@ -18,8 +16,6 @@ func init() { | |||
flag.Parse() | |||
} | |||
|
|||
r.SetVerbose(true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to remove setting verbose mode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-added
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why removing the random seed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would we remove initialCap
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because it was not used here
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
count is assigned in line 96, if already assigned in 86, its never used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented the review findings and commented the others
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because it was not used here
Reason for the change
With recent go versions slog comes as a decent logging implementation
slog
use 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.