fix unused linter errors#8851
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8851 +/- ##
==========================================
+ Coverage 51.12% 51.15% +0.03%
==========================================
Files 409 409
Lines 21369 21366 -3
==========================================
+ Hits 10925 10930 +5
+ Misses 9592 9584 -8
Partials 852 852 ☔ 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.
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.
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.
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.
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 ```
|
/test reconciler-tests |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: creydr, simkam The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Can be seen for example in: https://github.com/knative/eventing/actions/runs/21171606060/job/60889521311
/kind cleanup