chore(notify.go): Clean up duplicate code in RetryStage.exec

[rudo-thomas]

The code in the two selects in case ctx is done is exactly the same. Remove the second copy, the first one will be reached via the for loop.

[rudo-thomas]

Pinging @siavashs who seems to be active in this file :)

[siavashs]

First block: Non-blocking check at the start of each iteration to exit early if context is already done
Second block: Handles context cancellation while blocking on tick.C

Both code blocks are required and correct, therefor I'm closing this PR.

[rudo-thomas]

Let me clarify:

The second block does exactly(!) what the first one does.

Rather than copying the exact same non-trivial 12 lines of code to the second select's case, do nothing and the done context will be handled in the first select in the next iteration of the for loop.

I'm agruing this is more readable than copying the code, because it's not obvious it is exactly the same code. I guess a comment could be added to clarify.

@siavashs Please consired reopening, I'll add the comment if you think it makes things clearer.
Thanks.

[siavashs]

The second block does exactly(!) what the first one does.

It does the same thing but under different circumstances. So it is not exactly the same.

Furthermore, even if the code did the exact same thing, it is not causing an issue and is not a bug so there is no motivation to fix it.

I welcome you to look at existing open issues for more impactful contributions to the project.
I am looking forward to your future contributions.

Thank you

[siavashs]

Upon further discussion with other maintainers, the change is marked as valid.

[siavashs]

LGTM

[TheMeier]

I do think the code is actually correct as is. The second select is blocking where the first is not.
The first check prevents new notifications if the context is already cancelled, the second check allows cancellation while waiting for the next tick.

[rudo-thomas]

The code is correct as is, as well as after the proposed change.

All I'm trying to eliminate is the duplicated (copy-pasted) block: I'm arguing it's more readable without the copy.

[TheMeier]

Ok. I had a misread/misunderstanding there. Somehow I read this change as removing the case <- ctx.Done() entirely but instead the change just makes it a noop.

[rudo-thomas]

Can anyone with the privileges please merge this, now that it has been approved?

Or should I loop in anyone else?

Cheers.

[siavashs]

cc @SuperQ to run CI and merge