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
Add go-vet shadow variable checking. #2042
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2042 +/- ##
==========================================
+ Coverage 60.26% 60.29% +0.03%
==========================================
Files 229 230 +1
Lines 13331 13342 +11
==========================================
+ Hits 8033 8044 +11
Misses 4326 4326
Partials 972 972 |
scripts/pre-commit-go-vet
Outdated
# Get directory of script in order to get directory of project | ||
# This allows the script to work from any sub directory in the project | ||
DIR="$( cd "$( dirname "${BASH_SOURCE[0]}/" )" >/dev/null 2>&1 && pwd )" | ||
DIR="$(dirname "${DIR}")" |
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.
@chrisrcoles - this ought to fix the problem you saw today.
This works on experimental. But I saw a failure with the awardqueue and I'm worried about it. So I'm going to check that out first and try to find out what's up. |
This reverts commit ef699b6.
Subsequent reruns worked. Though please give a closer look if you can and try to catch anything I messed up. |
@@ -582,7 +582,7 @@ func showFunction(cmd *cobra.Command, args []string) error { | |||
} | |||
|
|||
if level := strings.ToLower(v.GetString(flagLogLevel)); len(level) > 0 { | |||
filters[logLevel] = level | |||
filters[filterLogLevel] = level |
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.
@pjdufour-truss - found this shadow variable and was hoping you'd take a look at it. I think I made the appropriate fix.
@akostibas - I keep seeing this problem with the award queue. Any idea what's up with this?
|
Turns out the awardqueue problem wasn't part of this PR. It was fixed in #2093 |
h.Logger().Error("DB Query", zap.Error(err)) | ||
tspUser, fetchTSPByUserErr := models.FetchTspUserByID(h.DB(), session.TspUserID) | ||
if fetchTSPByUserErr != nil { | ||
h.Logger().Error("DB Query", zap.Error(fetchTSPByUserErr)) | ||
// return tspop.NewGetTransporationServiceProviderForbidden() | ||
} |
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 some code below this that uses the err
but doesn't shadow. Your fix is fine, but just noting that it feels a little odd when we're using the err
from the earlier scope, but only on the second call in this block.
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.
go-vet isn't perfect, I'm pretty sure it just uses statistical modeling to find these things. But in at least a few cases it caught real problems with the code and I'm happy with that result.
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.
LGTM!
Description
We lost shadow variable checking when we moved from
go tool vet
togo vet
. This PR fixes that oversight and any missed vars.Reviewer Notes
Here are the problems I have to fix:
Setup
Add any steps or code to run in this section to help others prepare to run your code:
Code Review Verification Steps
References