Enhancement Add DisposeWith

[ChrisPulman]

Enhancement

Provides an extension to allow Disposables to be added to a collection of Disposables in a fluent manner.

A simple Test has been added to show its operation.

[ChrisPulman]

@idg10 If you are happy with this addition, I will fix the API tests, sorry I missed that.

[idg10]

If I've understood this correctly, the intention is not to add any new functionality. This is purely to enable a different way to express a thing we could already do.

So instead of writing compositeDisposable.Add(someDisposable) I can instead, if I prefer, write someDisposable.DisposeWith(compositeDisposable).

And I presume the main intended benefit of this is that DisposeWith is a direct expression of intent, making the code easier to understand than the more mechanism-focused Add.

I'm saying this to make sure I've not totally misunderstood the proposal.

I have two issues:

1. This feels like it's breaking new ground: introducing a particular opinion on coding style that Rx as not previously advocated for
2. This seems to let you call DisposeWith in (fairly simple) scenarios where your IDisposable will definitely not be disposed

Regarding 1, I personally happen to dislike it when libraries decide they are going to add a bunch of extension methods for some widely used type. I realise some people like to code that way and I have no wish to prevent them, but I don't like it when my autocomplete list grows a load of stuff that is irrelevant to what I'm trying to do because someone else decided they'd like a bunch of extension methods for string. It always strikes me as being rather rude.

IDisposable is a pretty significant type. If we were going to do this, I would absolutely want this to be in a distinct namespace. I personally wouldn't want this to appear in completion popups as a side effect of me deciding to use Rx's disposable types. I'd want it to be an opt-in thing. (I don't think I want this myself, but just as I object to other people imposing on me their ideas about what constitutes an appropriate number of extension methods to define for a widely-used type, I also shouldn't impose my view that '0' may well be the correct answer for types as critical as IDisposable and string. If others want this, I don't want to stop them. Putting this in a separate namespace, which I promise I won't insist on calling System.Reactive.Disposables.FillMyAutoCorrectWithAnnoyingJunk, would seem to serve both points of view.)

But 2 seems like a problem to me. I've explained my thinking in a comment in the review, attached to the relevant code.

[idg10]

Looking through Rx at all the other types that define extension methods, they tend to end in Extensions. E.g., TaskObservableExtensions, ObservableExtensions, VirtualTimeSchedulerExtensions.

So far we haven't used the terminology mixin at all.

Do you think this is a fundamentally different sort of thing from the existing things that use the Extensions suffix? If this has a different-looking name from other similar things, and if that signifies an important conceptual difference, then that's reasonable.

But if there is such a difference, I'm not sure what it is. Is there one? Or is this more of a naming preference thing? If so, how would you feel about aligning with existing conventions in this library here?

[idg10]

I'm slightly baffled now that I look at this. It appears to be ICollection.Add in disguise. And it does not appear to ensure that the IDisposable gets disposed.

For example, this seems to let me write the following:

List<IDisposable> disposables = new();
someDisposable.DisposeWith(disposables);

This is, as far as I can tell, exactly equivalent to:

List<IDisposable> disposables = new();
disposables.Add(someDisposable);

This DisposeWith method name is making promises it can't keep.

If there were something constraining the disposableCollection argument to be something that would actually dispose everything it contains, then this would make some sense. E.g. if its type were CompositeDisposable, then it would work, although even then, how much better is this:

someDisposable.DisposeWith(compositeDisposable);

than this:

compositeDisposable.Add(someDisposable);

Moreover, at that point it no longer works for anything other than CompositeDisposable in which case it might as well be a member of CompositeDisposable—there doesn't seem to be any value in it being an extension method. I presume that's why you chose the more general argument type of ICollection<IDisposable> (even though the XML doc comment says it's a "composite disposable".)

It looks like a basic presumption behind this method is that it's inconceivable that anything implementing ICollection<IDisposable> wouldn't dispose its contents. But that's untrue for List<IDisposable> (a type we use in several places internally in Rx).

I suppose it might be possible to constrain the second argument too, e.g. something like:

public static TResource DisposeWith<TResource, TCollection>(this T item, ICollection<IDisposable> disposableCollection)
    where TResource : IDisposable
    where TCollection : IDisposable

That still makes a fairly big assumption: that the collection type will dispose all of its contents. But I don't think that's necessarily true. If the collection type specifically implements ICollection<IDisposable> and IDisposable, then I'd go as far as to say that it seems quite likely that it would dispose each of its members. But if a collection type is generic, and just implements ICollection<T> (and IDisposable) and if it has not constrained T to be IDisposable, then I would expect it not to.

So here's how you'd actually want to constrain the collection type to be reasonably confident that DisposeWith would do what it promises:

• implements IDisposable
• implements ICollection<IDisposable>
• is not a generic type which, if instantiated with different type arguments, might have implemented ICollection<NotDisposable> instead

I don't believe it's possible to express that particular set of constraints in C#. So either you make this method so narrow that it is really just another name for CompositeDisposable.Add, or you leave it open in ways that means it won't necessarily function as advertise.

So, before we draw any conclusions from this analysis, do you think I've missed anything in the above?