Skip to content

Added IndexBy operator #562

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Jul 17, 2019
Merged

Added IndexBy operator #562

merged 13 commits into from
Jul 17, 2019

Conversation

@leandromoh
Copy link
Collaborator

need help with method summary/readme, please

@leandromoh
Copy link
Collaborator Author

@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
Copy link
Member

atifaziz commented Mar 7, 2019

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
Copy link
Collaborator Author

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 atifaziz changed the title Added IndexBy operator (issue #560) Added IndexBy operator Mar 13, 2019
@atifaziz
Copy link
Member

This PR addresses #560.

atifaziz
atifaziz requested changes Mar 13, 2019
View reviewed changes
@leandromoh
Copy link
Collaborator Author

@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
fsateler reviewed Mar 19, 2019
View reviewed changes
MoreLinq/IndexBy.cs Outdated
/// If null, the default equality comparer for <typeparamref name="TSource"/> is used.</param>
/// <returns>A sequence of unique keys and their number of occurrences in the original sequence.</returns>

public static IEnumerable<TResult> IndexBy<TSource, TKey, TResult>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@atifaziz atifaziz Mar 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 requested changes Mar 19, 2019
View reviewed changes
Copy link
Member

@atifaziz atifaziz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
atifaziz reviewed Mar 19, 2019
View reviewed changes
Copy link
Member

@atifaziz atifaziz left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 atifaziz mentioned this pull request Mar 19, 2019
Closed
@atifaziz
Copy link
Member

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
Copy link
Collaborator Author

@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
atifaziz requested changes May 21, 2019
View reviewed changes
Copy link
Member

@atifaziz atifaziz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator Author

@atifaziz adjusts done

atifaziz
atifaziz reviewed May 22, 2019
View reviewed changes
Copy link
Member

@atifaziz atifaziz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@atifaziz atifaziz merged commit 5205ea2 into morelinq:master Jul 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fsateler fsateler fsateler left review comments

@atifaziz atifaziz atifaziz approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@leandromoh @atifaziz @fsateler