IObservableList implements IReadOnlyList. ISourceList implements IList - #279
IObservableList implements IReadOnlyList. ISourceList implements IList#279Noggog wants to merge 5 commits into
Conversation
| return sources.Combine(CombineOperator.And); | ||
| } | ||
|
|
||
| /// <summary> |
There was a problem hiding this comment.
These functions are needed, as the two functions above them collide now that ISourceList implements them both. This function is a clarifying function that the compiler can use to remove the ambiguity. It internally calls the IObservableList version instead of the ICollection version.
| return sources.Combine(CombineOperator.Or); | ||
| } | ||
|
|
||
| /// <summary> |
There was a problem hiding this comment.
These functions are needed, as the two functions above them collide now that ISourceList implements them both. This function is a clarifying function that the compiler can use to remove the ambiguity. It internally calls the IObservableList version instead of the ICollection version.
| /// <summary> | ||
| /// Gets the count. | ||
| /// </summary> | ||
| int Count { get; } |
There was a problem hiding this comment.
This property was removed, as IReadOnlyList provides it now
| /// </summary> | ||
| /// <typeparam name="T"></typeparam> | ||
| public interface ISourceList<T> : IObservableList<T> | ||
| public interface ISourceList<T> : IObservableList<T>, IList<T> |
There was a problem hiding this comment.
I am not sure whether it is desirable to fully implement IList<T>. I have always resisted doing so since I actively want to discourage people thinking that it should be treated in the same way as a normal list.
The reason being that most list operations on source list are expensive in terms of inner locks and array copying. What I can be sure of is if consumers see Ilist then they will use it as a normal IList and this will surely lead to complaints about performance.
But then again maybe this is a needless worry of mine.
There was a problem hiding this comment.
I am interested in other opinions out there
There was a problem hiding this comment.
I'll just throw in my two cents, but I'm also interested in hearing some other's thoughts.
I agree with your sentiment that it's good to help guide people from using expensive objects in ways that feel cheap that might deceive them. I'm totally on board with that up until it starts to hinder other important patterns and goals. Lack of code re-usability is a large architectural concern for me compared to the upside of helping shepherd some newer users away from bad patterns.
Not to mention, I think the familiar IList traps are all still there via the SourceListEditConvenienceEx class, even if they don't "officially" come from an IList implementation. I know that I already personally have fallen into it not a few days ago, lol. I was populating a SourceList, and using Add() one by one like an idiot, which was astronomically slow. This is what eventually let me to create #277, as I opted to try out the BufferIf functionality rather than solving it using AddRange.
There was a problem hiding this comment.
Having sat on this one for a day, and looking at the code again I am beginning to think that there may be no harm in implementing IList, after all as you point out most of the methods were available on the convenience methods.
However I think it wise for me to sit on this for a few days more as once an API it is available I have to support it forever.
|
If we do do ilist we should maybe consider explicit implementation to avoid users from using the expensive operators. Then have a interface of common ones that are cheap. |
Now that SourceList implements IList, it now has some of the methods that the extensions were providing.
| // index is not a valid index in the ISourceList`1. | ||
| // | ||
| // T:System.NotSupportedException: | ||
| // The property is set and the ISourceList`1 is read-only. |
There was a problem hiding this comment.
Looks like something went wrong with the XML documentation, probably worth adjusting it a bit.
There was a problem hiding this comment.
I copied the comment from IList's comment for its indexer property directly, and just swapped IList for ISourceList. The odd looking `1 is in the original comments, too, as it's a result of how generics are toString()'ed by default. I do agree it's an odd way to display generics, but it is standard, I'd say. I don't mind swapping it out for something we all agree we like better, though.
There was a problem hiding this comment.
I think @worldbeater is referring it's not in xmldoc format with the /// and XML tags.
There was a problem hiding this comment.
In the current form it won't be picked up by intellisense.
There was a problem hiding this comment.
Ah, my bad. I'm following now.
Swapping it to the xml format. Thanks!
|
Spoken to @RolandPheasant we are inclined to decline this PR. The rationale is that it could have performance implications to the user if they start using it as a IReadOnlyList. The main concept of the SourceList/Cache is they are meant to be used binding to visible containers. |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
4 participants
What kind of change does this PR introduce?
IObservableList implements IReadOnlyList. ISourceList implements IList.
I find that I write a lot of convenience code via extension methods, and I try to hook them onto the most generic objects that I can. These would be functions similar to the ones found in DynamicData itself like
ListEx.AddRange(this IList<T> source, IEnumerable<T> items);Because DynamicData's list functionality didn't inherit from the List interfaces meant its collections didn't hook onto those extension methods. I had to duplicate the code for them to work with DynamicData, which is verbose and hindered refactorability as both versions needed to be maintained with the same updates.
I didn't see anything glaring as to why DynamicData's lists couldn't implement the interfaces, so I gave it a stab that seems to have worked out alright for my fork, so I thought I'd migrate it upwards for everyone else.
What is the current behavior?
SourceList and its interfaces implement list patterns, without actually implementing IList/IReadOnlyList
What is the new behavior?
SourceList and its interfaces implement IList/IReadOnlyList
What might this PR break?
This could break wiring that currently works around the fact that SourceList doesn't implement IList. I had to do a bit of maintenance myself on ObservableListEx, for example, adding another extension method that took ISourceList directly to clear the ambiguous call between the
IObservable<IChangeSet<T>>andICollection<IObservable<IChangeSet<T>>>versions.I believe I've handled all the cases in DynamicData itself, but users of the library might have made their own workarounds for the lack of IList that might need to be adjusted or dialed back.
Please check if the PR fulfills these requirements
Other information:
I ran the unit tests, which passed. I couldn't think of any obvious unit tests to add, but let me know if anyone thinks of some we could write to flex any of the new behavior.