-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Miscellaneous lint cleanup #7747
Conversation
mark-rushakoff
commented
Dec 19, 2016
•
edited
Loading
edited
- Rebased/mergable
- Tests pass
var out io.Writer = &e.LogOutput | ||
if testing.Verbose() { | ||
out = io.MultiWriter(out, os.Stderr) | ||
} | ||
e.QueryExecutor.WithLogger(zap.New( | ||
zap.NewTextEncoder(), | ||
zap.Output(os.Stderr), |
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.
What this supposed to be out
from L199?
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.
Probably. I'll amend the commit.
@@ -1208,7 +1208,7 @@ func (k *tsmKeyIterator) combine(dedup bool) blocks { | |||
} | |||
|
|||
func (k *tsmKeyIterator) chunk(dst blocks) blocks { | |||
for len(k.mergedValues) > k.size { | |||
if len(k.mergedValues) > k.size { |
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 for
since we are slicing values off on L1225 within the loop.
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.
There's no loop. There's a hardcoded return on the last 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.
Hmm. I'll have to look at when that changed. That may be a bug.
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 an if
as you have it. When the return was added, I forgot to switch it from a loop.
0b073d9
to
d4d5c91
Compare
@@ -87,7 +87,7 @@ func (c *MetaClientMock) Databases() []meta.DatabaseInfo { | |||
} | |||
|
|||
func (c *MetaClientMock) DeleteShardGroup(database string, policy string, id uint64) error { | |||
return c.DeleteShardGroup(database, policy, id) | |||
return c.DeleteShardGroupFn(database, policy, id) |
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.
Clearly this was never being called in a test 🤦♂️
d4d5c91
to
07b87f2
Compare
We should try and get this merged soon given it touches a lot of the code-base. |
Waiting on @jwilder to make a decision on the (non-)loop in |