AmpersandJS / ampersand-collection

A module for handling collections of objects
MIT License
68 stars 27 forks source link

Consistent sorting functions #59

Open amoskyler opened 9 years ago

amoskyler commented 9 years ago

ampersand-subcollection and ampersand-collection both use different sort methods.

Ampersand-subcollection uses lodash sortBy, and ampersand-collection uses native array sort. I ran into an issue where I was using the wrong sort method.

Not sure what the best change to make is, but maybe editing docs to make it more clear that collection/sub-collection have different APIs or normalizing the API to use the same sort method.

https://github.com/AmpersandJS/ampersand-subcollection/blob/master/ampersand-subcollection.js#L175

https://github.com/AmpersandJS/ampersand-collection/blob/master/ampersand-collection.js#L238

wraithgar commented 9 years ago

subcollection is being deprecated, see https://github.com/AmpersandJS/ampersand-subcollection/issues/43

This doesn't change this issue, just where you should be looking as we go forward

filtered-subcollection sortBy line is here https://github.com/AmpersandJS/ampersand-filtered-subcollection/blob/master/ampersand-filtered-subcollection.js#L259

amoskyler commented 9 years ago

What are your thoughts for a revision?

Modifying the APIs to both use the same sort function seems to make the most sense, but I could be missing something, in which case documentation clarity would be great.

I can go ahead and make the change and put a PR in whichever way seems to make sense.

wraithgar commented 9 years ago

I'd definitely want to hear from other @AmpersandJS/core-team members, but at first glance it seems to me the code in ampersand-collection is a rough version of what's in underscore/amp/lodash sortBy and we should switch to the latter so we are using an abstraction that's being independently maintained and tested.

amoskyler commented 9 years ago

Seems to make sense.