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

AR::Relation#order: make new order prepend old one. #7176

Merged
merged 1 commit into from
Jul 31, 2012

Conversation

bogdan
Copy link
Contributor

@bogdan bogdan commented Jul 27, 2012

Original PR and discussion #2008.

User.order("name asc").order("name desc")
    # SELECT * FROM users ORDER BY name desc, name asc

This also affects order defined in default_scope or any kind of associations.

cc @tenderlove

@al2o3cr
Copy link
Contributor

al2o3cr commented Jul 27, 2012

An incredibly minor issue, but is it possible to change the example in the comments / commit message to not use the same column twice? On a first read, that made me think this was specifically related to calling order with the same column more than once, which it's definitely not.

@rafaelfranca
Copy link
Member

Agree. Also you will need to rebase

@bogdan
Copy link
Contributor Author

bogdan commented Jul 29, 2012

Fixed comment. Rebased code.

@@ -319,6 +319,11 @@ def find_first
end
end

def default_scope_has_order?
default_scope = klass.send(:build_default_scope)
Copy link
Member

Choose a reason for hiding this comment

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

This send seems a smell. I think we should refactor to make this method public but part of our private API.

cc @tenderlove @jonleighton

Copy link
Member

Choose a reason for hiding this comment

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

also build_default_scope is not used in any place inside the framework. I don't know if it is there for historial reason.

Copy link
Member

Choose a reason for hiding this comment

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

The last usage of this method was removed @ 8572ae6

@bogdan
Copy link
Contributor Author

bogdan commented Jul 30, 2012

Fixed send(:build_default_scope) call.

@carlosantoniodasilva
Copy link
Member

"This pull request cannot be automatically merged." :D

    User.order("name asc").order("created_at desc")
    # SELECT * FROM users ORDER BY created_at desc, name asc

This also affects order defined in `default_scope` or any kind of associations.
@bogdan
Copy link
Contributor Author

bogdan commented Jul 31, 2012

rebased

rafaelfranca added a commit that referenced this pull request Jul 31, 2012
AR::Relation#order: make new order prepend old one.
@rafaelfranca rafaelfranca merged commit b09d6aa into rails:master Jul 31, 2012
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.

None yet

4 participants