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

Consistent sorting functions #59

Open
amoskyler opened this issue Apr 2, 2015 · 4 comments
Open

Consistent sorting functions #59

amoskyler opened this issue Apr 2, 2015 · 4 comments

Comments

@amoskyler
Copy link

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
Copy link
Contributor

subcollection is being deprecated, see AmpersandJS/ampersand-subcollection#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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

Seems to make sense.

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

No branches or pull requests

3 participants