NightOwl888 commented on issue #1059: URL: https://github.com/apache/lucenenet/issues/1059#issuecomment-2543455901
> > If you think about it, since we are having trouble testing it this way, it is very likely that users will need to be able to store these distinct types in a common variable type (after all, Java allows this), so making this a first class feature rather than a test mock is necessary. > > I'm not so sure it's necessary, this is only a problem with using the low-level Abstract*Collector types. I don't think many people are going to be using those low-level types (instead using the types in the GroupingSearch file), and those that are further still are probably not going to need to be switching between them in the same variable like the test does, and further still if they really need to do that, they can do a workaround like we're doing here. > > What I don't want to have to do is expose object-based properties that are shadowed in the generic classes (or worse, i.e. `ICollection<object> Groups` for a non-generic AbstractAllGroupsCollector base class). That seems like a cure that is worse than the disease, and I don't even know if it's feasible. > But it is how we do it elsewhere (for example, in [FieldComparer](https://github.com/apache/lucenenet/blob/master/src/Lucene.Net/Search/FieldComparator.cs#L252)). Having the ability to store a collection of `AbstractXXXCollector<T>` without knowing the generic closing type is a useful thing to have. Furthermore, it is supported by Java, so there may be examples on blogs and elsewhere that are doing it that way. That said, there doesn't seem to be a use case where it makes sense to do this for `AbstractAllGroupsCollector`. It looks like you neatly solved access to the `GroupCount` field using `IGroupCountCollector`, but the tests don't access the `Groups` property so there is no need to find a solution to cast it AFAICT. > > Would it make sense to add the closing types of IDictionary<TKey, TValue> to the classes that wrap them? It looks like we could cascade that down from AbstractGroupingSearch<T, TKey, TValue> down to the others like AbstractFirstPassGroupingCollector<T, TKey, TValue>, etc. > > No, because ValueSource implementations treat it as `IDictionary<object, object>`. Different implementations use different types for the keys and values. Object is the only common type. We also don't know which particular ValueSource implementations will be used, so that's pretty much impossible. This dictionary is truly just a bag of key/value pairs, strongly typing it would not help anything and would require a significant deviation from upstream. Gotcha. Perhaps we are better off sticking with `IDictionary`, then. This allows the user to either use `IDictionary<object, object>` or have it strongly typed without having to do some weird cast or conversion to pass it in. Need to think about this some more. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@lucenenet.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
