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

scopelint: Fix improper range loop var reference #14

Conversation

atc0005
Copy link
Collaborator

@atc0005 atc0005 commented Apr 8, 2020

Problem

Error from scopelint linter:

"Using a reference for the variable on range scope img"

Resolution

Use index from range loop to reach into the slice argument to retrieve the address of those values instead of using the range loop variable, which likely would have been a subtle source of bugs later on.

References

@atc0005 atc0005 requested a review from dasrick April 8, 2020 11:12
@atc0005
Copy link
Collaborator Author

atc0005 commented Apr 8, 2020

@dasrick I explicitly set you as a reviewer, but touched no other settings on this PR (e.g., adding labels) because we haven't yet discussed the intended scope of my involvement with this project (i.e., I did not want to make any assumptions).

messagecard.go Outdated Show resolved Hide resolved
@atc0005 atc0005 force-pushed the i6-fix-scopelint-issue-with-range-loop-var-ref branch from a29032e to 7550a17 Compare April 8, 2020 11:16
@atc0005
Copy link
Collaborator Author

atc0005 commented Apr 8, 2020

I remembered that Travis CI isn't running golangci-lint as intended (#13), so I ran it locally via make lint and saw the complaint about trailing whitespace. I've corrected that, rebased and force-pushed again; that linting error is no longer present.

Copy link
Owner

@dasrick dasrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks better than before and the linter is fine with it.

@atc0005 atc0005 force-pushed the i6-fix-scopelint-issue-with-range-loop-var-ref branch from 7550a17 to 605953a Compare April 9, 2020 09:28
Error from `scopelint` linter:

"Using a reference for the variable on range scope `img`"

Fix:

Use index from range loop to *reach into* the slice argument
to retrieve the address of those values instead of using the
range loop variable, which likely would have been a subtle
source of bugs later on.

refs #6, #11, #12
@atc0005 atc0005 force-pushed the i6-fix-scopelint-issue-with-range-loop-var-ref branch from 605953a to 83cf781 Compare April 9, 2020 09:34
@atc0005
Copy link
Collaborator Author

atc0005 commented Apr 9, 2020

@dasrick: Looks better than before and the linter is fine with it.

Thanks for looking over this. I rebased yet again when a local golangci-lint run complained about unnecessary leading whitespace. While I personally like leading/trailing whitespace, I understand that it can be frowned upon.

Once you've approved a PR like this one, do you want me to go ahead and merge it or leave for you to do so?

@dasrick
Copy link
Owner

dasrick commented Apr 9, 2020

Once you've approved a PR like this one, do you want me to go ahead and merge it or leave for you to do so?

If you are the one who created the PR and me the one who approved it, then you can merge it ... (since you have the access right to do so) ...

@atc0005
Copy link
Collaborator Author

atc0005 commented Apr 9, 2020

@dasrick: If you are the one who created the PR and me the one who approved it, then you can merge it ...

Acknowledged.

(since you have the access right to do so) ...

Sure. I just didn't want to make any assumptions, abuse granted access/trust, etc without talking to you about it first. Thanks for clarifying!

@atc0005 atc0005 self-assigned this Apr 9, 2020
@atc0005 atc0005 merged commit f640d50 into dasrick:master Apr 9, 2020
@atc0005 atc0005 deleted the i6-fix-scopelint-issue-with-range-loop-var-ref branch April 9, 2020 13:43
atc0005 added a commit to atc0005/send2teams that referenced this pull request Apr 11, 2020
Summary:

- Move linter choices from Makefile to separate include file

- Add scopelint linter to help catch variable use
  outside of intended scope

References:

- #42
- #44
- atc0005/todo#4
- dasrick/go-teams-notify#14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants