Allow SendOptions with delayed delivery constraints to be reused #5235
Conversation
|
Is there any way to cover this with a test? |
the extension methods are already under unit tests. Not sure this is worth an acceptance test? |
|
Was thinking about the case where the TM screws up the delivery constraints? |
|
@andreasohlund I've added an acceptance tests from the perspective of "defect driven testing". I think this leaves a certain gap of testing other send options about their reusability, but I guess it doesn't hurt to have this test. |
| @@ -20,12 +18,12 @@ public static void DelayDeliveryWith(this SendOptions options, TimeSpan delay) | |||
| Guard.AgainstNull(nameof(options), options); | |||
| Guard.AgainstNegative(nameof(delay), delay); | |||
|
|
|||
| if (options.GetExtensions().TryGetDeliveryConstraint(out DoNotDeliverBefore _)) | |||
| if (options.DelayedDeliveryConstraint is DoNotDeliverBefore) | |||
There was a problem hiding this comment.
Now that we aren't just adding delivery constraints to the bag for each call, we could remove this check. That means the last one called would be the one that gets used.
There was a problem hiding this comment.
Had a chat with @bording about this comment. It makes sense to get rid of these ifs.
There was a problem hiding this comment.
Though to be 100% completely safe, maybe we should keep them.
There was a problem hiding this comment.
Last one wins make sense to me. It could be argued that it would be a breaking change to remove but I can't see how a user could rely on us throwing so I'm 👍 to remove since that code will always be causing confusion and adding an issue to remove in v8 seems overkill?
There was a problem hiding this comment.
👎 for removing the check. I still think it makes sense to prevent setups like
options.DelayDeliveryWith(...)
options.DoNotDeliverBefore(...)
for calls to the same API, last-one-wins is already active.
There was a problem hiding this comment.
@timbussmann I also thought that we should guard against this scenario and throw early rather than users to discover they are doing something wrong much later in the game.
Assuming I'm sending messages and set delayed delivery constraint conditionally, I'd probably create two SendOptions and use accordingly. Will not call the two methods on the same object to alternate between delayed delivery constraints. What would be the use case where both options would be applied?
There was a problem hiding this comment.
What would be the use case where both options would be applied?
none. That would very likely be a mistake (maybe by reusing some code) which we should provide.
fixes #5147
ping @SeanFeldman