Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the IList from IReactiveList<T> #430

Merged
merged 1 commit into from
Dec 9, 2013

Conversation

haacked
Copy link
Contributor

@haacked haacked commented Dec 9, 2013

This should not have been on the generic interface. It only belongs to
the generic implementation.

Fixes #427

This should not have been on the generic interface. It only belongs to
the generic implementation.

Fixes #427
anaisbetts pushed a commit that referenced this pull request Dec 9, 2013
…ces-fix

Remove the IList from IReactiveList<T>
@anaisbetts anaisbetts merged commit 00a72d2 into master Dec 9, 2013
@anaisbetts
Copy link
Member

✨ ✨ ✨ ✨ ✨ ✨ ✨ ✨

@jlaanstra
Copy link
Member

Mmm, the fact that we lose ICollection looks strange to me.

Maybe we should make IReactiveCollection<T> no longer inherit IReactiveCollection?

IReactiveCollection can then inherit from ICollection. In addition we could add an IReactiveList interface that would extend IReactiveCollection with IList. This also solves the ambiguity and looks much cleaner to me.
It makes the interfaces look more like the ones already in the BCL.

That would result in this:

public interface IReactiveCollection : ICollection, IReactiveNotifyCollectionChanged, IReactiveNotifyCollectionItemChanged, INotifyPropertyChanging, INotifyPropertyChanged, IEnableLogger
{
}
public interface IReactiveCollection<T> : ICollection<T>, IReactiveNotifyCollectionChanged<T>, IReactiveNotifyCollectionItemChanged<T>, IEnableLogger
{
}
public interface IReactiveList<T> : IReactiveCollection<T>, IList<T>
{
}
public interface IReactiveList : IReactiveCollection, IList
{
}
public class ReactiveList<T> : IReactiveList<T>, IReactiveList, IReadOnlyReactiveList<T>
{
}

What do you think about this? I will create a pull request but I am curious what you guys think about this change before I create one.

@haacked
Copy link
Contributor Author

haacked commented Dec 10, 2013

Nah, there's a very good reason for this that actually maps to BCL types
well. I'll write up a longer explanation later.

It may well be the real "problem" is IReactiveCollection should be
IReactiveEnumerable.

On Tuesday, December 10, 2013, Johan Laanstra wrote:

Mmm, the fact that we lose ICollection looks strange to me.

Maybe we should make IReactiveCollection no longer inherit
IReactiveCollection?

IReactiveCollection can then inherit from ICollection. In addition we
could an IReactiveList interface that would extend IReactiveCollectionwith
IList. This also solves the ambiguity and looks much cleaner to me.
It also makes the interface look more like the ones already in the BCL.

That would result in this:

public interface IReactiveCollection : ICollection, IReactiveNotifyCollectionChanged, IReactiveNotifyCollectionItemChanged, INotifyPropertyChanging, INotifyPropertyChanged, IEnableLogger{}public interface IReactiveCollection : ICollection, IReactiveNotifyCollectionChanged, IReactiveNotifyCollectionItemChanged, IEnableLogger{}public interface IReactiveList : IReactiveCollection, IList{}public interface IReactiveList : IReactiveCollection, IList{}public class ReactiveList : IReactiveList, IReactiveList, IReadOnlyReactiveList{}

What do you think about this? I will create a pull request but I am
curious what you guys think about this change before I create one.


Reply to this email directly or view it on GitHubhttps://github.com//pull/430#issuecomment-30240523
.

@haacked
Copy link
Contributor Author

haacked commented Dec 10, 2013

Yeah, perhaps a better name for IReactiveCollection is IReactiveEnumerable. It serves as the base interface for reactive "collections".

In a way, this is analogous to how IReadOnlyCollection<T> doesn't implement ICollection and neither does ICollection<T>. This is because ICollection defines the Count property and so does IReadOnlyCollection.

However, if we could redo the base interfaces, I would probably make add an IReadOnlyCollection and have ICollection implement that and simply add the Add methods.

However, this wasn't done that way because the read only stuff came later.

Long story short, I think it'll be way more common that we'll have APIs that want to be able to handle any reactive collection (aka IReactiveCollection) than ICollection. Under your proposal, there's no base reactive collection interface. IReactiveCollection is that interface. As I mentioned before, perhaps a better name is IReactiveEnumerable to express that. I'm not opposed to that name change, I just don't feel strongly about it either way.

@haacked haacked deleted the haacked/reactive-list-interfaces-fix branch December 10, 2013 17:26
@jlaanstra
Copy link
Member

I modified my proposal and created a PR to move the discussion to its own thread #432.

@jlaanstra
Copy link
Member

In a way, this is analogous to how IReadOnlyCollection doesn't implement ICollection and neither does ICollection.

I was not aware of this, but it makes sense in that case.

It may well be the real "problem" is IReactiveCollection should be
IReactiveEnumerable.

The way I understand the readonly BCL interfaces naming, is that IReadOnlyEnumerable was a silly name and they therefore chose IReadOnlyCollection<T>. Because IReactiveCollection<T> implemented ICollection<T>, I thought of it as an ICollection type, which was clearly wrong. That is fixed in my PR #432, so I think it is fine to stick with IReactiveCollection<T>.

@haacked
Copy link
Contributor Author

haacked commented Dec 11, 2013

The way I understand the readonly BCL interfaces naming, is that IReadOnlyEnumerable was a silly name and they therefore chose IReadOnlyCollection<T>.

Heh, that was my reasoning as well for IReactiveCollection. :)

@lock lock bot locked and limited conversation to collaborators Jun 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Calling .Count or this[int indexer] on IReactiveList<T> is ambiguous
3 participants