-
Notifications
You must be signed in to change notification settings - Fork 103
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
Additional Code Linters #1304
Additional Code Linters #1304
Conversation
Signed-off-by: Ken Sipe <[email protected]>
Signed-off-by: Ken Sipe <[email protected]>
Signed-off-by: Ken Sipe <[email protected]>
Signed-off-by: Ken Sipe <[email protected]>
Signed-off-by: Ken Sipe <[email protected]>
Signed-off-by: Ken Sipe <[email protected]>
Signed-off-by: Ken Sipe <[email protected]>
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.
Yes, more linters please!
I'm already starting a discussion, questioning a duplicate checker with threshold of 400 lines :) But I agree with you that we should merge this first and later tighten the lll
and dupl
configurations and add the refactorings needed for that.
lll: | ||
line-length: 250 | ||
dupl: | ||
threshold: 400 |
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 seems like a very high threshold. The default is 15. I'm okay with having some code duplication but 400 lines seems like too much. WDYT?
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.
yeah... I do agree. This is what the controller/runtime project has... and tightening this results in work...
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 like it. And agreed, the dupl threshold should probably be lower, but we can tighten that up later
*dupl threshold that aligns with controller-runtime * lll as a linter with a line length limit of 250 Signed-off-by: Ken Sipe <[email protected]> Signed-off-by: Andreas Neumann <[email protected]>
*dupl threshold that aligns with controller-runtime * lll as a linter with a line length limit of 250 Signed-off-by: Ken Sipe <[email protected]> Signed-off-by: Thomas Runyon <[email protected]>
What this PR does / why we need it:
Enabling a number of additional linters which aligns with other related projects such as controlller-runtime. For controller-runtime these are being moved from command-line flags into the config file with PR https://github.com/kubernetes-sigs/controller-runtime/pull/773/files
The contentious configuration I imagine is line length limit of 250. controller-runtime uses 170 which seems reasonable. We have had no limit... the result of linting with 170 limit is large. Even with 250 limit there were 9 violations which are resolved in this PR. If it's ok with reviewers... lets debate the configurations after this PR. This PR at least put some checks and limits in place that previously didn't exist.
Fixes #