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

underscore max and min with empty collections #1513

Closed
j201 opened this Issue Jan 7, 2014 · 3 comments

Comments

Projects
None yet
3 participants
@j201
Contributor

j201 commented Jan 7, 2014

_.max<T> and _.min<T> return -Infinity and Infinity respectively if passed an empty collection, but both functions are annotated as having a return type T. So, if T isn't a number, then the given return type is incorrect. It would be a shame to change it to any, but without union types, I'm not sure there's another option.

@basarat

This comment has been minimized.

Show comment
Hide comment
@basarat

basarat Jan 7, 2014

Member

Union types only affect options interfaces. The thing you want should be possible via function overloading.

Member

basarat commented Jan 7, 2014

Union types only affect options interfaces. The thing you want should be possible via function overloading.

@j201

This comment has been minimized.

Show comment
Hide comment
@j201

j201 Jan 8, 2014

Contributor

I'm not sure function overloading would do it, but feel free to show me how if I'm wrong. Here's what I was thinking.

The current type for _.max<T> is this:

max(list: _.List<number>): number;
max<T>(list: _.Collection<T>, iterator?: _.ListIterator<T, any>, context?: any): T;

But if list is empty, then it will return -Infinity, regardless of what T is. So the correct type would be this:

max(list: _.List<number>): number;
max<T>(list: _.Collection<T>, iterator?: _.ListIterator<T, any>, context?: any): T | number;

But TypeScript doesn't support union types like that. If it kept track of whether a collection was empty or not, you could have an overload like this:

max(list: _.List<number>): number;
max<T>(list: _.EmptyCollection<T>, iterator?: _.ListIterator<T, any>, context?: any): number;
max<T>(list: _.Collection<T>, iterator?: _.ListIterator<T, any>, context?: any): T;

But it doesn't AFAIK. So the only correct option I can think of is this:

max(list: _.List<number>): number;
max<T>(list: _.Collection<T>, iterator?: _.ListIterator<T, any>, context?: any): any;
Contributor

j201 commented Jan 8, 2014

I'm not sure function overloading would do it, but feel free to show me how if I'm wrong. Here's what I was thinking.

The current type for _.max<T> is this:

max(list: _.List<number>): number;
max<T>(list: _.Collection<T>, iterator?: _.ListIterator<T, any>, context?: any): T;

But if list is empty, then it will return -Infinity, regardless of what T is. So the correct type would be this:

max(list: _.List<number>): number;
max<T>(list: _.Collection<T>, iterator?: _.ListIterator<T, any>, context?: any): T | number;

But TypeScript doesn't support union types like that. If it kept track of whether a collection was empty or not, you could have an overload like this:

max(list: _.List<number>): number;
max<T>(list: _.EmptyCollection<T>, iterator?: _.ListIterator<T, any>, context?: any): number;
max<T>(list: _.Collection<T>, iterator?: _.ListIterator<T, any>, context?: any): T;

But it doesn't AFAIK. So the only correct option I can think of is this:

max(list: _.List<number>): number;
max<T>(list: _.Collection<T>, iterator?: _.ListIterator<T, any>, context?: any): any;
@Igorbek

This comment has been minimized.

Show comment
Hide comment
@Igorbek

Igorbek Jan 11, 2014

Collaborator

@j201 using any instead of T is not a good idea. User expect typed value. Returning -Infinity is like an exception, so consumer of library should know about such behavior.
I close work item for now. Feel free to comment.

Collaborator

Igorbek commented Jan 11, 2014

@j201 using any instead of T is not a good idea. User expect typed value. Returning -Infinity is like an exception, so consumer of library should know about such behavior.
I close work item for now. Feel free to comment.

@Igorbek Igorbek closed this Jan 11, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment