danielwertheim / Ensure.That

Guard clause project for .NET
MIT License
439 stars 49 forks source link

Ensure.That(enumerable).HasItems() #148

Closed MadKillerChicken closed 3 years ago

MadKillerChicken commented 3 years ago

Hi,

maybe I'm just being dumb about it, but can someone please explain to me why the library doesn't implement the Ensure.That(x) flavor for IEnumerables? The only way of validating IEnumerables is by means of, for example, Ensure.Enumerables.HasItems(x).

Thanks!

danielwertheim commented 3 years ago

If I do recall it right, it was that you should be explicit in wanting to deal with Enumerables as there might e.g. be duplicate enumerations on the passed arg (other checks than null) which potentially could be a deferred/lazy evaluated construct doing something costly.

Hope that answers your question. Feel free to re-open otherwise.

ndrwrbgs commented 3 years ago

Thanks for opening your first issue in an OSS project, and welcome to the OSS community @MadKillerChicken!

I see two topics here. The first is, as Daniel mentions, the discouragement and "why you shouldn't" on this API. The second is the inconsistency you noticed. I include a discussion below on "Mini IEnumerable warning" for cases to keep in mind as you're validating IEnumerables, and the kinds of cases that would influence this design decision.

Identification and discussion of Ensure.That opportunities

Daniel, I think we should take one of two actions here. Either lean hard into "opinionated" and remove (Obsolete to not be breaking, and to advise folks in their IDEs) all of the EnumerableArg methods, or ensure functionality parity between the different flavors.

Do nothing

Good

Bad

Bad

Bad

_[0] Fancy Enumerables While I cannot do the same for SizeIs, HasAny has a completely valid, and safe, use case in the case of Memoization [Documentation]. Since a Memoized IEnumerable replays played elements, anything observed will always be reproducible, and anything observed will not be wasted (presuming it's being used again later, which, if it's not, why are we validated HasAny anyway). While I love thinking about the "theory" and "edge cases", I've not seen Memoization in practice in the wild, and as such do not think a design decision ought to be made on that possibility._

Opinions

I think we should deprecate each method on EnumerableArg with an Obsolete tag with a message pointing to the Collection methods and either a short description for why to avoid the obsolete enumerable-based methods, or pointing to a url resource with the same.

Mini IEnumerable warning

I apologize if I waste your time explaining anything you already know, I want to make sure we're on the same page for the solution here. Also, as the author of another library specifically dealing with IEnumerable, I get excited at the opportunity to discuss it.

Any time I think of IEnumerable and what to do with them, and in particular for this question, I think of misbehaving IEnumerables. C# has a weird identity problem with IEnumerable, especially with LINQ popularity and how it takes IEnumerable as input for everything. IEnumerable just represents something that "can be enumerated". This is why it just has a GetEnumerator method, which in turn just has Current and MoveNext. A big thing to note here is that it does not have any contract that it ever terminates (Case 1), that it is reproducible (returns the same values every time) (Case 2), nor that it is cheap (it can [and sadly, often will] be doing network calls) (Case 3). Often times C# developers (myself included!) will use IEnumerable, either as a return type (to promise the least to our consumers and enable future changes) or as an input type (because "I'm just using a LINQ method and that's the type it requires!") in cases that, conceptually, we might actually mean another type.

IEnumerable forever side note

Unfortunately, we'll be staying in this world for quite some time as the type we often probably mean (unless it's a return value from a network service that really CAN be infinite or costly, and in those cases should be handled as such) would be a Collection type. A way to say "I have some items, no, I really have them, right here, already". The reason we'll be stuck where we are for a while is that ICollection in C# as you can see does a lot more than say "I have items, right here" (notably: it's mutable and copyable). One might then say IReadOnlyCollection, however, due to when that was introduced (and that it's an interface that actually says what it CAN'T do, rather than CAN, but let's put that aside as List : IReadOnlyCollection even though it's writable) ICollection is NOT IReadOnlyCollection. Many(/most?) types that are ICollection currently are also IReadOnlyCollection, and that's useful, but this lack of ICollection : IReadOnlyCollection means that any API that took in an ICollection argument, cannot pass it to method that takes in IReadOnlyCollection.

Case 1: Never terminates

Traditionally, I've seen the case that an IEnumerable can be infinite usually be ignored, and so I think that's okay to do when it's not part of your expectations. I still think it's important to be aware of the contract of the values you're taking in, for the times when you CAN be prepared for them.

Contrived code example

IEnumerable<int> Forever() {
  while (true) yield return 1;
}

Real examples

Case 2: Non-reproducible

Contrived code example

IEnumerable<int> EmptyHalfOfTheTime() {
  if (new Random().NextDouble() < 0.5) yield return 1;
}

Real examples

Case 3: Non-cheap

Contrived code example

IEnumerable<int> Expensive() {
  for (int i = 0; i < 10; i ++) {
    Thread.Sleep(1000);
    yield return i;
  }
}

Real examples

MadKillerChicken commented 3 years ago

@danielwertheim @ndrwrbgs Thank you both very much for the info and especially for this project.