Code simplification #2516
Code simplification #2516
Conversation
provider/docker/docker.go
Outdated
| return | ||
| } | ||
| for range stop { | ||
| cancel() |
There was a problem hiding this comment.
Hmm, is this really exactly equivalent for the case where the channel gets closed? Doesn't the old version still call cancel() while the new one doesn't anymore?
Also, couldn't we just do
<-stop
cancel()as we immediately return anyway when the channel fires?
There was a problem hiding this comment.
My understanding here was that we need to cancel if we take stop. Refactoring should be keeping semantics.
Only <-stop will call anyway so it's wrong to me.
There was a problem hiding this comment.
Inside the goroutine, we select on the stop channel without any other select case available. That means we can only ever escape the select when the stop channel fires (either through a message or a close). Once it does, we invoke cancel and return immediately, giving us zero chance to have another for loop iteration that could process any further events from the stop channel (which makes total sense as we want to process the stop signal exactly once).
I think my simplification suggestion does effectively the same, using just fewer keywords: wait for a channel event; call cancel; and then return (implicitly). Please let me know where you see the semantical difference.
Should be good though to have another pair of eyes on this one.
There was a problem hiding this comment.
Since it was different from other cases bottom, I thought intention was to call only on getting something on channel. Otherwise, semantic will be kept same with just <-stop and actually putting for range won't call cancel if we just close channel. Updating this part and rebasing as requested by @ldez.
ldez
added
status/2-needs-review
kind/enhancement
|
@ferhatelmas could you rebase ? |
|
@timoreimann @ldez rebased and diff got even smaller 😄 |
- drop `select` with only one `case`
|
Updated additional notes to reflect last status. |
What does this PR do?
Does some simplification.
Motivation
Makes code easier to understand since uses less tokens.
More
Additional Notes
selectwith only onecase