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

Remove WhereChain, and implement where_not. #9551

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
5 participants
@ernie
Copy link
Contributor

commented Mar 5, 2013

While the WhereChain implementation made some sense when we were
supporting where.not, where.like, and where.not_like, it doesn't
really make sense when supporting only the where.not case.

This commit removes WhereChain and implements where_not, instead. While
the method-chaining version might seem a bit prettier (I do tend to
think a dot is more attractive than an underscore) it also incurs extra
method call overhead and makes the implementation more complex to read,
as well.

Remove WhereChain, and implement where_not.
While the WhereChain implementation made some sense when we were
supporting where.not, where.like, and where.not_like, it doesn't
really make sense when supporting only the where.not case.

This commit removes WhereChain and implements where_not, instead. While
the method-chaining version might seem a bit prettier (I do tend to
think a dot is more attractive than an underscore) it also incurs extra
method call overhead and makes the implementation more complex to read,
as well.
@carlosantoniodasilva

This comment has been minimized.

Copy link
Member

commented Mar 5, 2013

It seems reasonable to me as well since we only have where.not now, but we need to hear more thoughts here. Also, I think a changelog entry would have to be added talking about the modification from beta1? Thanks @ernie.

@ernie

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2013

@carlosantoniodasilva One of these days I will remember we're maintaining the CHANGELOG more religiously now. That day was not today. Fixed.

Yeah, I hate the idea of modifying something like this late in the game, but it is a pretty simple change from a user standpoint, and cleans up the implementation quite a bit. Wish I'd had time to write this up earlier, before the beta went out.

@rafaelfranca

This comment has been minimized.

Copy link
Member

commented Mar 5, 2013

@jeremy since you proposed the API I think you should give your thoughts here.

cc @dhh

@jeremy

This comment has been minimized.

Copy link
Member

commented Mar 5, 2013

Good instinct, @ernie. Only not made the initial cut, but the essential pattern for building up a where clause is good. We'll leave that ground for third-party libs to cover with like, or, regexp, etc, and see what crops up. Other operations may make the cut for core in future releases.

@jeremy jeremy closed this Mar 5, 2013

@ernie

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2013

@jeremy thanks for taking a look. Are you sure there isn't room for a bit more discussion on this one before closing it?

Leaving aside the discussion of whether where.<predicate> is a good idea for the API for a moment, I'm concerned that we're locking ourselves in to a specific implementation before it's needed. It would be easy to at some point alter a where_not implementation to use a WhereChain and transition people to where.not should that API design prove useful for other predicates in the long run. As it sits, we're adding complexity to the where method, adding another class, and sending to private methods on the parent in order to make it all work, for something that people might extend in the future.

Back in the AR 3.x days, we supported only equality (and, for certain value types, IN/BETWEEN). There was a good deal of discussion around what a more full-featured API for creating other ARel predicates might look like, among them @lifo's SuperCondition implementation, which formed the basis for MetaWhere at the time. The suggestion at the time was that we would take a look at what plugins were developed and seemed to be most useful and at some point in the future consider selecting one for integration into core.

Here we are, 3 years later, and as best I can tell that isn't what is happening here. I haven't seen the implementation we've gone ahead with extensively tested in plugin form, but we're integrating it all the same. As such, I'm disappointed to see additional complexity in an implementation with the promise of some potential future ecosystem developing around it driving additions to core, when that has historically not been the case for the AR query API.

Given that I have skin in the game, here, I should clarify that I'm not necessarily saying that we need to implement Squeel's version of predicate building. I know it may not be for everyone. But I am saying that I built both MetaWhere and Squeel as proofs-of-concept based on the expectation that they would be included in the running for an eventual enhanced query builder API, and they have received far more real-world usage than the API we've landed on.

My last attempt at driving some discussion around this was at https://groups.google.com/d/msg/rubyonrails-core/evUmld2Mal4/PGFLjX9ZeNkJ and met with a lackluster response.

To summarize, these are the drawbacks I see with this approach at first glance:

  • Alteration of types (both duck- and actual) throughout a method chain, making order of messages being sent important where it previously was not
  • Existing methods for relation composition map closely to the query domain, mapping to specific clauses of the query for this most part. where.not (or, for that matter, where_not) breaks this trend
  • Should the pattern be extended to other predicates, we find ourselves chunking our where clauses by type of predicate instead of by related concept or attribute, or reverting back to string queries. The likelihood that scopes will hide this fact from typical application use cases does not prevent it from having a certain smell.
  • The implementation as it sits today is awkward (injecting the previous scope, sending to its private build_where method, and branching based on a number of possible return types then mutating and returning the injected scope), and we should stop to consider whether the feature envy is telling us something.
@ernie

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2013

No? Anyone?

@bf4

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2013

bump

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.