Added IndexBy operator

[leandromoh]

need help with method summary/readme, please

[leandromoh]

@atifaziz on AppVeyor build I am receiving "New code was generated during build that's not been committed." error. I belive that it is about generated extension wrappers. Can you help me please?

[atifaziz]

AppVeyor build I am receiving "New code was generated during build that's not been committed." error. I belive that it is about generated extension wrappers. Can you help me please?

That's correct. You just need to run build.cmd or build.sh locally before doing a commit (which is what I did with c215552). Those scripts will always make sure that the generated code is up to date. Doing a build in an IDE like Visual Studio or Visual Studio Code isn't enough.

[leandromoh]

Nice. When I tried to run these cmd I got an error and the console closed before I could read. May we follow with the review?

[atifaziz]

This PR addresses #560.

[leandromoh]

@atifaziz adjusts done. Except by elements. I did it for consistency, IndexBy need elements, CountBy does not, but CountByImpl does not know them, so it always provides.

[fsateler]

I feel like IndexBy is not very descriptive of what this does. Other languages like Ruby have index_by as what C# knows as ToDictionary. I'm having a hard time coming up with a better name. Since this is mostly CountBy, where the last parameter to resultSelector is decremented once, does this really make sense to add?

[atifaziz]

I feel like IndexBy is not very descriptive of what this does. Other languages like Ruby have index_by as what C# knows as ToDictionary. I'm having a hard time coming up with a better name.

It's consistent with Index, which pairs each element of sequence with its zero-based position in the sequence. IndexBy does the same except the positions are relative to a key group. That's how the name makes sense to me; that it's a variation of Index.

Since this is mostly CountBy, where the last parameter to resultSelector is decremented once, does this really make sense to add?

CountBy aggregates on keys. This gives you the source elements in the result so is different enough to be useful.

[atifaziz]

@atifaziz adjusts done.

Thanks 👍

Except by elements. I did it for consistency, IndexBy need elements, CountBy does not, but CountByImpl does not know them, so it always provides.

I understand but now CountBy will pay the price for something not requested by the user, for some internal implementation detail of code sharing. We should resolve this somehow.

[atifaziz]

I feel like IndexBy is not very descriptive of what this does. Other languages like Ruby have index_by as what C# knows as ToDictionary. I'm having a hard time coming up with a better name. Since this is mostly CountBy, where the last parameter to resultSelector is decremented once, does this really make sense to add?

The fact that you're having trouble with the name got me thinking some more. It seems to me, what we may want to have here in the end is something more general, a ScanBy (perhaps the name you were looking for?) whereby IndexBy wouldn't be needed anymore. It would also enable a whole other set of running (grouped) accumulations. It might also explain why it's odd to have IndexBy piggy-backing on CountBy's implementation.

---

PS See #563 for the idea.

[atifaziz]

I did a quick spike and implemented IndexBy in terms of ScanBy with 16f2cb4. In terms of design, I believe we should keep IndexBy as simple as Index. In other words, it shouldn't return any user-defined projection, but just key-value pairs where the key is the index within the group. The only downside is that it can be somewhat confusing to have a by operator where the returned key is something other than what keySelector returns but I imagine that wouldn't be too surprising either since this is just another flavour of Index.

[leandromoh]

@atifaziz glad you figure out the method benefícits. The implantation looks great. About no materialize entire source, we should add a ‘*DoesNotIterateUnnecessaryElements’ test to assure It.

[atifaziz]

The CountBy implementation changes in this PR should be reverted now that IndexBy uses ScanBy.

we should add a ‘*DoesNotIterateUnnecessaryElements’ test to assure It.

Good idea!

[leandromoh]

@atifaziz adjusts done

[atifaziz]

Thanks. I need to review the documentation whenever I have time next.