Consider changing `minWith` and `maxWith` APIs to be more like `sortBy` #65

Closed
CrossEye opened this Issue Jun 21, 2014 · 29 comments

Comments

Projects
None yet
3 participants
@CrossEye
Member

CrossEye commented Jun 21, 2014

sortBy has a nicer API than do minWith and maxWith.

In sortBy we generate a key for the objects to sort, and the comparator to use is automatically created based on this key. So we can do something as simple as

sortBy(prop('title'), list);

minWith and maxWith, by contrast, which don't even need to be sorted, require you to supply a comparator.

minWith(function(a, b) {return a.x - b.x;}, points)

This really should be

minWith(prop('x'), points);

Granted the current API offers some theoretic additional flexibility. But I can't see any realistic cases that would take advantage of it.

If there are no objections, I'll do this over the weekend.

@CrossEye CrossEye self-assigned this Jun 21, 2014

@buzzdecafe

This comment has been minimized.

Show comment
Hide comment
Member

buzzdecafe commented Jun 21, 2014

+1

@CrossEye

This comment has been minimized.

Show comment
Hide comment
@CrossEye

CrossEye Jun 21, 2014

Member

Fixed in c1c986b

Member

CrossEye commented Jun 21, 2014

Fixed in c1c986b

@CrossEye CrossEye closed this Jun 21, 2014

@megawac

This comment has been minimized.

Show comment
Hide comment
@megawac

megawac Jun 22, 2014

Contributor

I think this should this be done with uniqWith, differenceWith, et. al. as well

Contributor

megawac commented Jun 22, 2014

I think this should this be done with uniqWith, differenceWith, et. al. as well

@buzzdecafe

This comment has been minimized.

Show comment
Hide comment
@buzzdecafe

buzzdecafe Jun 22, 2014

Member

i'm changing my mind on this. what if the objects in the list you are evaluating have complicated relations? e.g. uniqWith(function(a, b) { return a.x === b.y && a.y === b.y; }, list) does not get captured using this approach. Then the simple prop implementation of the *With functions is a special case of passing a more abstract comparator.

Member

buzzdecafe commented Jun 22, 2014

i'm changing my mind on this. what if the objects in the list you are evaluating have complicated relations? e.g. uniqWith(function(a, b) { return a.x === b.y && a.y === b.y; }, list) does not get captured using this approach. Then the simple prop implementation of the *With functions is a special case of passing a more abstract comparator.

@buzzdecafe buzzdecafe reopened this Jun 22, 2014

@CrossEye

This comment has been minimized.

Show comment
Hide comment
@CrossEye

CrossEye Jun 22, 2014

Member

EDIT: this was in response to @megawac's comment, not @buzzdecafe's.

I'm not so sure about that. I can easily see:

MyType.areEqual(a, b) {
    if (!(a instanceof MyType && b instanceof MyType)) {return false;}
    if (a.hash() !== b.hash()) {return false;}
    if (a.testDifficultProperty1() !== b.testDifficultProperty1()) {return false;}
    if (a.testDifficultProperty2() !== b.testDifficultProperty2()) {return false;}
    // ...
    return true;
}

uniqWith(MyType.areEqual, listOfMyType)

This can't necessarily be captured by a single key.

Member

CrossEye commented Jun 22, 2014

EDIT: this was in response to @megawac's comment, not @buzzdecafe's.

I'm not so sure about that. I can easily see:

MyType.areEqual(a, b) {
    if (!(a instanceof MyType && b instanceof MyType)) {return false;}
    if (a.hash() !== b.hash()) {return false;}
    if (a.testDifficultProperty1() !== b.testDifficultProperty1()) {return false;}
    if (a.testDifficultProperty2() !== b.testDifficultProperty2()) {return false;}
    // ...
    return true;
}

uniqWith(MyType.areEqual, listOfMyType)

This can't necessarily be captured by a single key.

@megawac

This comment has been minimized.

Show comment
Hide comment
@megawac

megawac Jun 22, 2014

Contributor

Some other libraries allow you to return an array from these functions which will compare in order.

Contributor

megawac commented Jun 22, 2014

Some other libraries allow you to return an array from these functions which will compare in order.

@CrossEye

This comment has been minimized.

Show comment
Hide comment
@CrossEye

CrossEye Jun 22, 2014

Member

@buzzdecafe: I agree regarding uniqWith, et al. But I can't see any use cases for minWith / maxWith, although maybe we should rename to minBy / maxBy ?

Member

CrossEye commented Jun 22, 2014

@buzzdecafe: I agree regarding uniqWith, et al. But I can't see any use cases for minWith / maxWith, although maybe we should rename to minBy / maxBy ?

@buzzdecafe

This comment has been minimized.

Show comment
Hide comment
@buzzdecafe

buzzdecafe Jun 22, 2014

Member

i was also thinking *By for single-property comparison

Member

buzzdecafe commented Jun 22, 2014

i was also thinking *By for single-property comparison

@CrossEye

This comment has been minimized.

Show comment
Hide comment
@CrossEye

CrossEye Jun 22, 2014

Member

@buzzdecafe I'm also not sure that your example makes real sense, as I think all of these comparisons really need to be symmetric in the two parameters. But the general point stands.

But yes, use *By for single-property comparisons, whether a natural property of the object or a synthesized one, *With for a more general function. That would be a cleaner API.

Member

CrossEye commented Jun 22, 2014

@buzzdecafe I'm also not sure that your example makes real sense, as I think all of these comparisons really need to be symmetric in the two parameters. But the general point stands.

But yes, use *By for single-property comparisons, whether a natural property of the object or a synthesized one, *With for a more general function. That would be a cleaner API.

@buzzdecafe

This comment has been minimized.

Show comment
Hide comment
@buzzdecafe

buzzdecafe Jun 22, 2014

Member

I think the objection stands for minWith on the same grounds. You can have a list of objects that have some crazy rules for determining which one is the smallest that is not captured in an attribute. Imagine there is some kind of pecking order ...

Member

buzzdecafe commented Jun 22, 2014

I think the objection stands for minWith on the same grounds. You can have a list of objects that have some crazy rules for determining which one is the smallest that is not captured in an attribute. Imagine there is some kind of pecking order ...

@CrossEye

This comment has been minimized.

Show comment
Hide comment
@CrossEye

CrossEye Jun 22, 2014

Member

@megawac Don't understand about the array. I'm mostly ok with composeing reverse if necessary. How does returning an array help?

Member

CrossEye commented Jun 22, 2014

@megawac Don't understand about the array. I'm mostly ok with composeing reverse if necessary. How does returning an array help?

@megawac

This comment has been minimized.

Show comment
Hide comment
@megawac

megawac Jun 22, 2014

Contributor

Would give you the same functionality from a *By function as a *With

ramda.maxBy(function(x) {
   return [x.x, x.y]
}, [{x: 1, y: 2}, {x: 1, y: 1}, {x: 1, y: 5}, {x: 1, y: 2}]); //=> {x: 1, y: 5}

Contributor

megawac commented Jun 22, 2014

Would give you the same functionality from a *By function as a *With

ramda.maxBy(function(x) {
   return [x.x, x.y]
}, [{x: 1, y: 2}, {x: 1, y: 1}, {x: 1, y: 5}, {x: 1, y: 2}]); //=> {x: 1, y: 5}

@buzzdecafe

This comment has been minimized.

Show comment
Hide comment
@buzzdecafe

buzzdecafe Jun 22, 2014

Member

@CrossEye. that is essentially the way i implemented the path* functions. In that case, should the sig of *By be Str -> [Obj] -> Obj? and the inital Str gets converted to prop(Str)?

@megawac ok, that's interesting, but does not address the more complex example @CrossEye suggested

Member

buzzdecafe commented Jun 22, 2014

@CrossEye. that is essentially the way i implemented the path* functions. In that case, should the sig of *By be Str -> [Obj] -> Obj? and the inital Str gets converted to prop(Str)?

@megawac ok, that's interesting, but does not address the more complex example @CrossEye suggested

@CrossEye

This comment has been minimized.

Show comment
Hide comment
@CrossEye

CrossEye Jun 22, 2014

Member

@megawac Got it; thought you were talking ascending vs descending. I'd rather not go there. If we get as complicated as that, I'd rather just let 'em use an arbitrary function.

Member

CrossEye commented Jun 22, 2014

@megawac Got it; thought you were talking ascending vs descending. I'd rather not go there. If we get as complicated as that, I'd rather just let 'em use an arbitrary function.

@megawac

This comment has been minimized.

Show comment
Hide comment
@megawac

megawac Jun 22, 2014

Contributor

Alright, it might be worth creating a *By generator and a *With generator and allowing users to decide regardless (because personally I hate having to work with a and b all the time [begins to look confusing])

Contributor

megawac commented Jun 22, 2014

Alright, it might be worth creating a *By generator and a *With generator and allowing users to decide regardless (because personally I hate having to work with a and b all the time [begins to look confusing])

@CrossEye

This comment has been minimized.

Show comment
Hide comment
@CrossEye

CrossEye Jun 22, 2014

Member

@buzzdecafe I like a more general *By for these. The key could still be entirely synthetic:

var makeSortName = fork(join(' '), prop('lastName'), always(','), prop('firstName'));
maxBy(makeSortName, people);

(untested, and don't know if we actually have join, but should be clear.)

Member

CrossEye commented Jun 22, 2014

@buzzdecafe I like a more general *By for these. The key could still be entirely synthetic:

var makeSortName = fork(join(' '), prop('lastName'), always(','), prop('firstName'));
maxBy(makeSortName, people);

(untested, and don't know if we actually have join, but should be clear.)

@CrossEye

This comment has been minimized.

Show comment
Hide comment
@CrossEye

CrossEye Jun 22, 2014

Member

@megawac I agree, and it should be easy to implement the *By in terms of the *With.

Member

CrossEye commented Jun 22, 2014

@megawac I agree, and it should be easy to implement the *By in terms of the *With.

@buzzdecafe

This comment has been minimized.

Show comment
Hide comment
@buzzdecafe

buzzdecafe Jun 22, 2014

Member

@CrossEye that example seems like a maxWith to me. What is the distinction of By and With in your mind?

btw, gotta come up with a new name for fork

Member

buzzdecafe commented Jun 22, 2014

@CrossEye that example seems like a maxWith to me. What is the distinction of By and With in your mind?

btw, gotta come up with a new name for fork

@CrossEye

This comment has been minimized.

Show comment
Hide comment
@CrossEye

CrossEye Jun 22, 2014

Member

@buzzdecafe *By focuses on a single list item, generates a key for it. *With is the one that knows how to do the comparison between two different items. That was the issue that started me down this trail. There was no good reason to be writing a full-fledged comparator when I often wanted to either fetch or synthesize something from a single object.

Member

CrossEye commented Jun 22, 2014

@buzzdecafe *By focuses on a single list item, generates a key for it. *With is the one that knows how to do the comparison between two different items. That was the issue that started me down this trail. There was no good reason to be writing a full-fledged comparator when I often wanted to either fetch or synthesize something from a single object.

@buzzdecafe

This comment has been minimized.

Show comment
Hide comment
@buzzdecafe

buzzdecafe Jun 22, 2014

Member

at present pathBy takes a string as its first param. I'd like all of the *By fns to have the same semantics, and passing a function is overkill to pathBy. And I also don't wanna type-check the first arg

Member

buzzdecafe commented Jun 22, 2014

at present pathBy takes a string as its first param. I'd like all of the *By fns to have the same semantics, and passing a function is overkill to pathBy. And I also don't wanna type-check the first arg

@CrossEye

This comment has been minimized.

Show comment
Hide comment
@CrossEye

CrossEye Jun 22, 2014

Member

Yeah, that's a different beast, isn't it? I've been thinking about max/min/sort/uniq/difference, which I think could all share these semantics. But path is definitely different.

Member

CrossEye commented Jun 22, 2014

Yeah, that's a different beast, isn't it? I've been thinking about max/min/sort/uniq/difference, which I think could all share these semantics. But path is definitely different.

@megawac

This comment has been minimized.

Show comment
Hide comment
@megawac

megawac Jun 22, 2014

Contributor

I disagree @buzzdecafe, I think a lookupIterator function would be sweet for choosing how to use the given argument

Contributor

megawac commented Jun 22, 2014

I disagree @buzzdecafe, I think a lookupIterator function would be sweet for choosing how to use the given argument

@CrossEye

This comment has been minimized.

Show comment
Hide comment
@CrossEye

CrossEye Jun 22, 2014

Member

@megawac Yes, the trouble is that it would be nice if we could have a universal semantic for the *By and *With, which just got different meanings on the new path.

Member

CrossEye commented Jun 22, 2014

@megawac Yes, the trouble is that it would be nice if we could have a universal semantic for the *By and *With, which just got different meanings on the new path.

@megawac

This comment has been minimized.

Show comment
Hide comment
@megawac

megawac Jun 22, 2014

Contributor

It wouldn't be hard to mix that in with an iterator

Contributor

megawac commented Jun 22, 2014

It wouldn't be hard to mix that in with an iterator

@buzzdecafe

This comment has been minimized.

Show comment
Hide comment
@buzzdecafe

buzzdecafe Jun 22, 2014

Member

since pathBy seems to be the problem child here, maybe the simplest solution is to come up with a new name for it.

Member

buzzdecafe commented Jun 22, 2014

since pathBy seems to be the problem child here, maybe the simplest solution is to come up with a new name for it.

@buzzdecafe

This comment has been minimized.

Show comment
Hide comment
@buzzdecafe

buzzdecafe Jun 22, 2014

Member

let's rename pathBy to pathOn and use With and By semantics elsewhere as described above.

Member

buzzdecafe commented Jun 22, 2014

let's rename pathBy to pathOn and use With and By semantics elsewhere as described above.

@CrossEye

This comment has been minimized.

Show comment
Hide comment
@CrossEye

CrossEye Jun 22, 2014

Member

@megawac: We've been trying to avoid any unnecessary type-checking, making our functions as strongly-typed as feasible in JS. We definitely aren't interested in the Underscore/LoDash style of "if parameter 3 is a function do x, if it's an object, do y, and if it's a string do z." This is not to say that we have no type-checking, but there's relatively little, and that's mostly to support dynamic dispatch for extensions, such as the lazy lists, and (soon, I hope) the algebraic types.

@buzzdecafe That might be best. But can't think of a good name.

Member

CrossEye commented Jun 22, 2014

@megawac: We've been trying to avoid any unnecessary type-checking, making our functions as strongly-typed as feasible in JS. We definitely aren't interested in the Underscore/LoDash style of "if parameter 3 is a function do x, if it's an object, do y, and if it's a string do z." This is not to say that we have no type-checking, but there's relatively little, and that's mostly to support dynamic dispatch for extensions, such as the lazy lists, and (soon, I hope) the algebraic types.

@buzzdecafe That might be best. But can't think of a good name.

@CrossEye

This comment has been minimized.

Show comment
Hide comment
@CrossEye

CrossEye Jun 22, 2014

Member

@buzzdecafe That sounds good.

Member

CrossEye commented Jun 22, 2014

@buzzdecafe That sounds good.

@CrossEye

This comment has been minimized.

Show comment
Hide comment
@CrossEye

CrossEye Jul 11, 2014

Member

The original goal of this issue was long ago satisfied. If we want to follow up on the remaining discussion, we can reopen later.

Member

CrossEye commented Jul 11, 2014

The original goal of this issue was long ago satisfied. If we want to follow up on the remaining discussion, we can reopen later.

GingerPlusPlus pushed a commit to GingerPlusPlus/ramda that referenced this issue Feb 24, 2018

@char0n char0n referenced this issue in char0n/ramda-adjunct Apr 22, 2018

Open

RA.overWhen / RA.overUnless #349

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