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

Add array.aggregate function #535

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

igitur
Copy link
Contributor

@igitur igitur commented Feb 16, 2024

Fixes #532

Instead of a reduce function, I implemented array.aggregate, that also accept an initial seed value. I could add more tests if you want to. Just point me to the applicable spot in the test suite where this would be required.

@igitur
Copy link
Contributor Author

igitur commented Feb 16, 2024

And if you're happy with this approach, I'd like to implement more LINQ-ish array functions. array.sum, array.average, array.all, array.any, array.min, array.max and maybe array.groupby.

@xoofx
Copy link
Member

xoofx commented Feb 16, 2024

Oh definitely, you can add them as part of this PR

@r-Larch
Copy link
Contributor

r-Larch commented Feb 16, 2024

Hi,

I really appreciate these additions; great work!

I reviewed the array.aggregate implementation and believe it could be improved by making the seed value optional. Making the seed value optional can enhance flexibility and usability for cases where the initial value isn't necessary or when the user prefers the aggregation to start directly with the elements of the array. Specifically, if no seed value is provided, it would be beneficial to automatically use the first item of the list as the current value and the second item as the next value. This approach simplifies the function's usage for common aggregation tasks, such as summing values or concatenating strings, where the natural starting point is the first element of the array. It also aligns with the behavior of similar functions in other programming languages, making it more intuitive for users transitioning from different backgrounds.

What do you think?

@igitur
Copy link
Contributor Author

igitur commented Feb 16, 2024

@r-Larch I played around with that idea. The order if the 2 parameters would need to swap from seed & function to function and optional seed. Right? But when I tried that with the seed provided, the function consumes the seed instead of it being passed into array.aggregate. Not sure if it's a bug or by design.

@igitur
Copy link
Contributor Author

igitur commented Feb 16, 2024

Maybe just easier to implement array.reduce, which wouldn't have the seed parameter.

@r-Larch
Copy link
Contributor

r-Larch commented Mar 18, 2024

@r-Larch I played around with that idea. The order if the 2 parameters would need to swap from seed & function to function and optional seed. Right? But when I tried that with the seed provided, the function consumes the seed instead of it being passed into array.aggregate. Not sure if it's a bug or by design.

Hmm.. for me it looks like a bug but I don't know.

Does it work with parameters like this: (seed optional before func)

Method Desc
array.aggregate <func(c, n)> no seed, just a func param
array.aggregate <seed> <func(c, n)> seed + func

In C#

public static object Aggregate(TemplateContext context, SourceSpan span, IEnumerable list, object? arg1, object? arg2);
if (arg1 is IScriptCustomFunction) ...
if (arg2 is IScriptCustomFunction) ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Array reduction
3 participants