- Notifications
You must be signed in to change notification settings - Fork1k
chore: Remove varnamelen linter#854
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Emyrk commentedApr 4, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Also the linter doesn't say where the lint failure is"
https://github.com/coder/coder/runs/5818950685?check_suite_focus=true |
codecovbot commentedApr 4, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## main #854 +/- ##==========================================- Coverage 66.08% 65.87% -0.21%========================================== Files 202 202 Lines 13209 13209 Branches 87 87 ==========================================- Hits 8729 8702 -27- Misses 3597 3619 +22- Partials 883 888 +5
Continue to review full report at Codecov.
|
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've never felt like our variable names in v1 were too short in my opinion.
I've had this bite me a few times, I think it's too sensitive. I agree with it in theory but it wouldn't let me use a single character variable for a 4 line middleware function. Idiomatic Go would say that is totally fine as long as it's not a super long function and doesn't leave scope.
I generally don't love using # of lines as the metric for this.
I'd prefer for us organic life forms to make the decision on this sort of thing. |
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've personally found this to be quite helpful in producing readable code. I think that edge-case of specific types matched with specific names can be helpful to declare.
It's an arbitrary position, but I'd rather have consistency withrw http.ResponseWriter
over a mix ofw
andrw
than the shorter name.
My comment doesn't need to block merge. Mostly everyone agrees, so if it becomes a problem we can add it back. |
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.
Didn't realize my request for change would block merge. Since everyone on the team seems to agree with this, I shall approve too.
Golang prefers short var names in local scope. I think this is not idiomatic and excessive. I'd only assert this when the var exceeds local scope, but that is not supported.
https://github.com/golang/go/wiki/CodeReviewComments#variable-names