Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Allow reversal of orderings #64

Merged
merged 1 commit into from Jun 21, 2011

Conversation

Projects
None yet
2 participants
Collaborator

ernie commented Jun 14, 2011

Hey @tenderlove, what do you think of something along these lines to simplify an Ordering-respecting reverse_sql_order over in query_methods.rb? Noticed a previous patch was rejected due to to_sql'ing the nodes.

Owner

tenderlove commented Jun 15, 2011

Why change the symbols to numbers? I'd like to keep the symbols for backwards compat (I think). Otherwise, yes, this seems good.

Collaborator

ernie commented Jun 15, 2011

Just to avoid branching on a reverse, really. There's just something
about being able to do -direction that's nice, and I figured people
were using the factory methods and ascending?/descending? as the API
so it shouldn't matter.

I can certainly rework with the symbols though.

On Jun 15, 2011, at 3:12 AM, tenderlove
reply@reply.github.com
wrote:

Why change the symbols to numbers? I'd like to keep the symbols for backwards compat (I think). Otherwise, yes, this seems good.

Reply to this email directly or view it on GitHub:
#64 (comment)

Owner

tenderlove commented Jun 15, 2011

Ya, avoiding the branch is nice. I don't really like that the Ordering object can mutate it's internals (via reverse!).

Can we just change reverse_sql_order to update the AST with a new node rather than mutate the existing object?

Also, I'm wondering why we don't have specific ASC and DESC nodes? Then we could avoid branching.

wdyt?

Collaborator

ernie commented Jun 15, 2011

Yeah, not married to the mutator here. Almost didn't include it since
I didn't remember seeing any elsewhere. It'll probably just trip
someone else up down the line.

Something like Unary > Ordering > Ascending/Descending? I'd still like
to keep a reverse factory method around on both so that
reverse_sql_order reads relatively cleanly.

Part of me sort of thinks AR should be coercing params to #order into
a proper ordering node to begin with, so reverse_sql_order would look
like:

orders.map(&:reverse)

Instead of all the string hackery. That's probably too heavy-handed though. :(

On Jun 15, 2011, at 6:45 AM, tenderlove
reply@reply.github.com
wrote:

Ya, avoiding the branch is nice. I don't really like that the Ordering object can mutate it's internals (via reverse!).

Can we just change reverse_sql_order to update the AST with a new node rather than mutate the existing object?

Also, I'm wondering why we don't have specific ASC and DESC nodes? Then we could avoid branching.

wdyt?

Reply to this email directly or view it on GitHub:
#64 (comment)

Collaborator

ernie commented Jun 15, 2011

How's this strike you? About as backwards-compat as I think we could be, by keeping the old direction/ascending?/descending? methods

Collaborator

ernie commented Jun 20, 2011

Sorry to be a nuisance, but does anyone have any input on the changed commit?

Owner

tenderlove commented Jun 21, 2011

Hey Ernie, sorry to take so long to het back to you. These changes look good to me. I will merge them in tomorrow when I've had a little more sleep.

Thanks.

Aaron Patterson
http://tenderlovemaking.com/

On Jun 20, 2011, at 6:07 AM, erniereply@reply.github.com wrote:

Sorry to be a nuisance, but does anyone have any input on the changed commit?

Reply to this email directly or view it on GitHub:
#64 (comment)

Owner

tenderlove commented Jun 21, 2011

Yay! Looks awesome. Merging now. ❤️❤️❤️❤️❤️❤️❤️

@tenderlove tenderlove added a commit that referenced this pull request Jun 21, 2011

@tenderlove tenderlove Merge pull request #64 from ernie/reverse_ordering
Allow reversal of orderings
645afa0

@tenderlove tenderlove merged commit 645afa0 into rails:master Jun 21, 2011

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