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 a sum() method to Array #75939

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

Conversation

Burstwright
Copy link
Contributor

I think it seems reasonable to have this built-in, since it's such a common task. The implementation's similar to min() and max(), so I placed it next to those in the code.

@Burstwright Burstwright requested review from a team as code owners April 11, 2023 15:50
@AThousandShips
Copy link
Member

AThousandShips commented Apr 11, 2023

This is easily achieved with reduce which is exactly what that is for, I don't think this is all that required, see godotengine/godot-proposals#3776

It creates a bit of bloat in the engine, if we add sum we also need to add product, etc.

@Burstwright
Copy link
Contributor Author

True, but the example given in the docs, Array.reduce(func(accum, number): return accum + number), seems a bit verbose to me. In terms of future bloat, Python has a built-in sum(), but no product(), so I'm not sure that's an inevitable outcome.

I understand if this doesn't get merged, though. It's not hard to implement in GDScript, but it seemed common enough to warrant a method.

@phylor
Copy link

phylor commented Apr 11, 2023

Especially for beginners sum() is way easier to understand and read then Array.reduce(func(accum, number): return accum + number). The latter needs quite some mental capacity to understand what it does.

Most other languages I work with have a builtin sum, so I'd really appreciate it if GDScript had it as well! 🙏

@AThousandShips
Copy link
Member

I don't necessarily agree with that "it's good for beginners" is a good argument for adding a feature that can easily be achieved with existing functionally

@MewPurPur
Copy link
Contributor

MewPurPur commented Apr 11, 2023

I think we should keep Array an all-purpose container for variants, not favoring summable types for any of its methods.

The linked proposal doesn't argue well. Their use case is "I need it" and "If I do it in GDScript it'll be slow" -_-. Also parallels with Python don't work here because this isn't about syntax; we have our own unique variants and most of them can't be summed together. Meanwhile, reduce() can perfectly replicate what sum() could do in a slightly longer line of code, and it also fulfills other use cases, such as finding min or max and using custom methods of summing or comparing.

@Calinou Calinou added this to the 4.x milestone Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants