Skip to content
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

fix shadowed variables that were preventing errors from being returned properly #1030

Merged
merged 1 commit into from Apr 14, 2022

Conversation

danbrakeley
Copy link
Contributor

@danbrakeley danbrakeley commented Nov 19, 2021

I pulled latest and opened it up in VS Code, and there were 3 ineffectual assignment to err (ineffassign) errors from the go-golangci-lint linter, found in restapi.go on line 2548, state.go on line 857, and state.go on line 873.

The "ineffectual assignment to err" means that the code is assigning a value to a variable err, but then the variable goes out of scope without ever being used. Looking at the code, all 3 cases are due to the := assignment operator creating a second err variable in the local scope, instead of re-using the err that is a named return value.

More info: Golang Variable Shadowing.

@FedorLap2006
Copy link
Collaborator

Oh, wow, I totally forgot about this while I was writing that function, thank you.

@CarsonHoffman
Copy link
Collaborator

CarsonHoffman commented Nov 25, 2021

I'm not sure that the changes in state.go actually do anything—yes, we're shadowing err, but the returns immediately after those calls then refer to the inner err, no? (They're return err, not return.) Shadowing err is a common and typically innocuous practice. I agree that the InteractionRespond case is an issue.

@danbrakeley
Copy link
Contributor Author

danbrakeley commented Nov 25, 2021

@CarsonHoffman So in those two cases in state.go, you are right, the code behaves the same with this PR. But if you look at the rest of the function, my changes make those cases consistent with the rest of the code. And it removes linter errors, which means the linter output will be cleaner and easier to spot real issues. Also, removing the shadowing now means less chance of a change in the future causing problems.

the linter caught shadowed variables that were preventing errors from being returned properly
@FedorLap2006 FedorLap2006 added this to the v0.25.0 milestone Apr 6, 2022
@FedorLap2006 FedorLap2006 added the fix Pull requests and issues related to bug fixes and structural inconsistencies label Apr 6, 2022
@FedorLap2006
Copy link
Collaborator

Thanks for your contribution!

@FedorLap2006 FedorLap2006 merged commit 2b35977 into bwmarrin:master Apr 14, 2022
@danbrakeley danbrakeley deleted the linter branch April 14, 2022 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Pull requests and issues related to bug fixes and structural inconsistencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants