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

feat(collections): add sumOf #1108

Merged
merged 14 commits into from
Aug 9, 2021
Merged

feat(collections): add sumOf #1108

merged 14 commits into from
Aug 9, 2021

Conversation

grian32
Copy link
Contributor

@grian32 grian32 commented Aug 7, 2021

Not sure if there should be more tests, couldn't really think of anything else as its a pretty simple function.

Related: #1065

collections/sum_of.ts Outdated Show resolved Hide resolved
collections/sum_of.ts Outdated Show resolved Hide resolved
collections/sum_of.ts Outdated Show resolved Hide resolved
collections/sum_of_test.ts Show resolved Hide resolved
collections/sum_of.ts Outdated Show resolved Hide resolved
@LionC
Copy link
Contributor

LionC commented Aug 7, 2021

Totally unrelated question now that I am seeing this implemented - should we maybe support bigint?

Edit: As I understand it, bigint supports + without weird caveats, so you would "just" need to add it to the typing of the function to support it. Sounds like an easy win?

@grian32
Copy link
Contributor Author

grian32 commented Aug 8, 2021

Totally unrelated question now that I am seeing this implemented - should we maybe support bigint?

Edit: As I understand it, bigint supports + without weird caveats, so you would "just" need to add it to the typing of the function to support it. Sounds like an easy win?

I'd need some extra checks so sum could start off as bigint instead of number, not sure if that'd hurt performance but I'll push it eitherway. We can benchmark after if it's needed.

Edit: Actually, I'm not really sure how to implement this. as I cant get typescript to really agree since the selector would be a union type and it doesn't like adding that to bigint or number even if i have typeguards

@caspervonb
Copy link
Contributor

Edit: Actually, I'm not really sure how to implement this. as I cant get typescript to really agree since the selector would be a union type and it doesn't like adding that to bigint or number even if i have typeguards

Would have to make it generic:

function sumOf<T>(array: T[]): T;

Keep in mind that you can't add a number and a bigint. You'll have to explicitly convert or get a runtime error.

@LionC
Copy link
Contributor

LionC commented Aug 8, 2021

As the type is determined by the selector, you could probably use

S extends (el: T) => number | (el: T) => bigint

as the type for selector and ReturnType<S> as the return type for the whole function.

The only problem is the empty input case - it needs to return 0 but you cannot know if that is bigint or number 0...

@kt3k kt3k mentioned this pull request Aug 9, 2021
26 tasks
@grian32
Copy link
Contributor Author

grian32 commented Aug 9, 2021

I don't think I'll be implementing bigint support in this PR as I don't know how to implement it type wise and the actual implementation would be scuffed enough that it'd just be wasteful in terms of memory and performance. @LionC @caspervonb But other than that I think it's good to go

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

@grian32 LGTM. I think this is enough for the first pass. Thank you for your contribution!

@kt3k kt3k merged commit adb7652 into denoland:main Aug 9, 2021
@kt3k
Copy link
Member

kt3k commented Aug 9, 2021

With regard to bigint support, how about providing explicit option or alternative function?

sumOf(arr, func, { bigInt: true });
sumOfBigInt(arr, func);

@LionC
Copy link
Contributor

LionC commented Aug 9, 2021

With regard to bigint support, how about providing explicit option or alternative function?


sumOf(arr, func, { bigInt: true });

sumOfBigInt(arr, func);

I think this is actually the only way to do this with the current signature - I do not see how we can decide which 0 to return for an empty input array.

That is something for another day then :-) Thank you a lot @grian32 , good work!

@LionC LionC mentioned this pull request Aug 27, 2021
44 tasks
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.

4 participants