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

Allow passing string hash values to AR order method #10732

Conversation

marceloeloelo
Copy link
Contributor

The order method from AR supports different kinds of arguments. When using a hash as the parameter this is now supported:

Topic.order(id: :desc)

but this is NOT:

Topic.order(id: 'desc')

I think this is not the behavior that a developer would expect from a method like order, so I added a few changes to support that kind of capabilities.

IMHO, this can be useful when passing 'ASC' or 'DESC' directly from the request params, like for a basic list sorting, without the need of making conversions to symbols, which can be annoying or unclean in some situations...

end

def test_nothing_raises_on_upcase_desc_arg
assert_nothing_raised { Topic.order(:id => "DESC") }
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't we also verify that it's sorting the right way and not just that it does not raise an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That should be tested in "test_finding_with_assoc_order_with_string". However, I modified the test a bit so it's more exact...

@senny
Copy link
Member

senny commented Jul 12, 2013

I think this behavior is reasonable. I added a few minor comments and you also need to add a CHANGELOG entry.

@marceloeloelo
Copy link
Contributor Author

Added CHANGELOG, changed syntax notation and added some more tests.

@robin850
Copy link
Member

@marceloeloelo : Could you please rebase this pull request? This doesn't merge cleanly.

@marceloeloelo
Copy link
Contributor Author

@robin850 rebased!

@@ -1,3 +1,11 @@
* Allow passing string values to AR order method:
Example:
Copy link
Member

Choose a reason for hiding this comment

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

Please add a blank line before Example:

@senny
Copy link
Member

senny commented Oct 28, 2013

@pftg if I remember correctly you sent a PR about that part of the code. Is this patch still needed on master?

@pftg
Copy link
Contributor

pftg commented Oct 28, 2013

@senny I think that this patch is fully works, but there are some places after validating order args (at least in method reverse_sql_order) where used Symbols: :asc and :desc. To be consistent, they should be String too.

@senny
Copy link
Member

senny commented Nov 5, 2013

can someone push a rebased and updated version of this PR?

@jhephs
Copy link

jhephs commented Mar 3, 2014

I hope this gets fixed before Rails 4.1.0 is officially released. This is very helpful actually. 👍

Also, if I may add, is there any publicly accessible rails constant out there which contains an array of :asc and :desc? I think this can be helpful for the views to determine which order to use.

@robin850 robin850 modified the milestones: 4.2.0, 4.0.4 Mar 3, 2014
senny added a commit that referenced this pull request Mar 5, 2014
…_hash

Follow up of #10732 - Allow string hash values on AR order method
@robin850 robin850 removed this from the 4.0.4 milestone Mar 5, 2014
@robin850
Copy link
Member

robin850 commented Mar 5, 2014

I think we can close this as #14263 has been merged and @senny improved it through f6aeb8b and b74eed5. Thank you guys!

@robin850 robin850 closed this Mar 5, 2014
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.

5 participants