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

Use activerecord reorder method instead of order #15

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

KRaikk
Copy link

@KRaikk KRaikk commented Jun 15, 2018

Order queries with ActiveRecord's reorder method to force ordering by given array as collection can prepend an order:

class User < ActiveRecord::Base
...
scope :latest, -> { order created_at: :desc }
names = %w(Pearl John)

User.latest.pluck(:name)
# => ['John', 'Kathenrie', 'Pearl']

User.latest.with_order(:name, names).pluck(:name)
# With current code 'latest' scope's order will execute first:
# => ['John', 'Kathenrie', 'Pearl']

# With this pull request, reorder will force ordering by given 'names' array:
# => ['Pearl', 'John', 'Kathenrie']

@khiav223577
Copy link
Owner

Thanks for you PR.

It would be better to make this feature optional since these changes will change original behavior. There are also some cases that may not want to replace existing orders. For example, I want to order the players by their score and if the score is the same, order by given school names.

players.order(:score).with_order(:school_name, school_names)

It would also be nice to have some test cases for the new feature :).

@@ -9,12 +9,14 @@

Find records in the same order of input array.

Forked from [khiav223577/find_with_order](https://github.com/khiav223577/find_with_order)
Copy link
Owner

@khiav223577 khiav223577 Jun 15, 2018

Choose a reason for hiding this comment

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

The changes made to README.md is not suitable for this PR.

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.

2 participants