Fixing code style #377
Fixing code style #377
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci-robot
added
the
approved
| t.Fatalf("the expected intervals didn't match actual intervals. \nExpected %v \nActual %v", setIntervals, actualIntervals) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Why are you removing all this? I know it's ignored, but I don't see any harm in leaving it there.
There was a problem hiding this comment.
Unused code should be always removed, we are using version control to handle it to us. So, no need to keep it in the repository, a few enumerated reasons related to code quality:
- "Out of sight" is also "Out of mind";
- Reduce the code base size;
- Version control handles it and keeps it forever;
- Lint will always complain about unused functions, variables, etc;
There are also few articles about it:
https://www.infoq.com/news/2017/02/dead-code/
https://blog.ndepend.com/no-excuse-dead-code/
https://www.oreilly.com/library/view/becoming-a-better/9781491905562/ch04.html
There was a problem hiding this comment.
OK then it would be good to fix the test or at least create some tracking issue, because "Out of sight" is also "Out of mind" as you said :)
There was a problem hiding this comment.
I'll create an issue to review this test.
| // just to make lint happy | ||
| if cm != nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
I don't think this is correct. I think we want to continue in the test when cm != nil.
There was a problem hiding this comment.
Oh, wait! Wrong operator. 🤦
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
|
+1 from me, love to see the removal of dead code. Also the consistency the linting will bring will be a breath of fresh air. :) /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 0sewa0, rluders The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This PR fixes some code styling issues, based on the linting tool report.
Categories
Sample archive
No.
Documentation
No.
Unit Tests
It doesn't include any tests, but most lint issues were related to it.
Privacy
Yes. There are no sensitive data in the newly collected information.
Changelog
No.
References
https://issues.redhat.com/browse/CCXDEV-4079