search

Azure / azure-service-bus-dotnet

☁️ .NET Standard client library for Azure Service Bus
https://azure.microsoft.com/services/service-bus
Other
235 stars 120 forks source link

Allow sending empty collection by skipping the actual send #642

Closed SeanFeldman closed 5 years ago

SeanFeldman commented 5 years ago

Fixes #550

SeanFeldman commented 5 years ago

@nemakam please review

SeanFeldman commented 5 years ago

Marked PR as [WIP] until it's not and ready to be merged.

dlstucki commented 5 years ago

@nemakam, et. al., what's the explanation for not throwing ArgumentException instead? Is it ease of the implementation of this library method? Most APIs I can think of which have some kind of "Send" method tend to throw if you tell it to send but don't give any data for sending. I feel like silently doing nothing might be hiding errors in the code calling this library.

If it's ease of implementation (like avoiding enumerating something twice) could something like this be done?

SendBatch(IEnumerable<Whatever> batch)
{
    bool foundAny = false;             // *****
    foreach (Whatever w in batch)
    {
        foundAny = true;                 // *****
        // ... Do something ...
    }

    if (!foundAny) { throw new ArgumentException("batch", "Batch contained no items"); } // *****
}
SeanFeldman commented 5 years ago

@dlstucki that was my initial take, but there was a compelling argument why an empty list should not throw: https://github.com/Azure/azure-service-bus-dotnet/issues/550#issuecomment-461330073

dlstucki commented 5 years ago

@dlstucki that was my initial take

@SeanFeldman Thanks! Seeing other APIs which behave this way makes me feel better.

SeanFeldman commented 5 years ago

The only reason this is still not merged is because I have no idea what would be a better way to test this behaviour w/o hitting the broker. If you have ideas @dlstucki, please share 🙂

dlstucki commented 5 years ago

@SeanFeldman, I haven't been following along very closely so I probably don't understand it. What do you need to verify in order to test this? Would this work?

SeanFeldman commented 5 years ago

@dlstucki that's sort of in place already.

SeanFeldman commented 5 years ago

I'll go with this for now and if a better test idea emerges, can be PR-ed