-
Notifications
You must be signed in to change notification settings - Fork 618
fix unused linter errors #8851
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
base: main
Are you sure you want to change the base?
fix unused linter errors #8851
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: simkam The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8851 +/- ##
==========================================
+ Coverage 51.13% 51.15% +0.01%
==========================================
Files 409 409
Lines 21369 21366 -3
==========================================
+ Hits 10928 10930 +2
+ Misses 9588 9584 -4
+ Partials 853 852 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/retest-required |
| // } | ||
| // t.Fatalf("FAIL: No traceid in %d messages: (%v)", len(evInfos), evInfos) | ||
| // return "" | ||
| // } |
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 suggest to not commit commented code with a TODO because it won't be picked up. It's better to remove this entire section and replace it with a comment with a reference to an upstream help-wanted ticket which describes why this was commented and what to do about it.
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.
The thing is that the code with TODO is already commented (https://github.com/knative/eventing/blob/main/test/conformance/helpers/tracing_test_helper.go#L59) but there are one const definition and one function definition that are used by only that commented code and due to that the unused linting error is triggered. I only commented pieces that are used by that already commented code.
I would rather not remove code that is supposed to be used somewhere. I don't haven't any context why that piece of code was commented.
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.
The thing is that the code with TODO is already commented...
Yep, I am aware of that. My intention was to motivate a small clean up of what was there and needs a cleanup because you touch the code in that area anyway. And the reason for the proposed cleanup is what you said a few sentences later =>
I would rather not remove code that is supposed to be used somewhere. I don't haven't any context why that piece of code was commented.
You describe the problem right here perfectly 😆 . That is the reason to not comment and commit code, because nobody will remember why this was done and will be afraid to remove it. For that reason a ticket with a reference should be the way to go. If you don't want to remove it, it's ok but maybe create a ticket and reference it there and the ticket should be "investigate if the commented code is needed or can safely be removed" so it has the chance of being picked up.
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.
ok, I agree. I logged #8853 and removed the commented code altogether.
Can be seen for example in: https://github.com/knative/eventing/actions/runs/21171606060/job/60889521311 ``` Error: pkg/requestreply/ingress_handler.go:352:6: func isResponseEvent is unused (unused) func isResponseEvent(event *cloudevents.Event, rr *v1alpha1.RequestReply) bool { ^ Error: test/conformance/helpers/tracing_test_helper.go:53:3: const recordEventsPodName is unused (unused) recordEventsPodName = "recordevents" ^ Error: test/conformance/helpers/tracing_test_helper.go:98:6: func getTraceIDHeader is unused (unused) func getTraceIDHeader(t *testing.T, evInfos []recordevents.EventInfo) string { ^ 3 issues: * unused: 3 ```
Can be seen for example in: https://github.com/knative/eventing/actions/runs/21171606060/job/60889521311
/kind cleanup