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

Don't require dm-aggregates in will_paginate/data_mapper #262

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants

reidab commented Aug 30, 2012

Not all DM adapters implement the necessary interface for the dm-aggregates plugin, but may instead implement a #count method by other means. Since will_paginate really only needs a working #count method on the DM collection to function, let DM users require dm-aggregates in their own code if they want it.

I've also added a require of dm-aggregates to the DM test connector to maintain the present behavior of that test.

Remove dependency on dm-aggregates in will_paginate/data_mapper
Not all DM adapters implement the necessary interface for the
dm-aggregates plugin, but may instead implement a #count method by
other means. Since will_paginate really only needs a working #count
method on the DM collection to function, let DM users require
dm-aggregates in their own code if they want it.

This pull request fails (merged e81f257 into 71f793e).

reidab commented Aug 30, 2012

I'm failing to reproduce the failure travis is reporting locally using 1.8.7 and the Rails 3.0 gemfile. It's a failure related to AR and posgres, so I'd be surprised if this change affected it.

Owner

mislav commented Sep 16, 2013

If I start requiring people to manually require "dm-aggreggates", that will break their existing apps. I think it's ok for will_paginate to require it if it needs its functionality. Can "dm-aggregates" break anyone's code?

reidab commented Sep 16, 2013

This is no longer a problem in our codebase, as we've moved away from using DM in the way that was causing our problem. Feel free to close this issue.

Here's a little more context about the edge case that caused me to submit it in the first place, in case anyone else is hitting it:

It broke our code, but I suspect our code was somewhat on the weird side. We were using a fairly minimal custom DM adapter, descending from AbstractAdapter, to talk to an internal API. Our adapter defined a "count" method which made the proper API calls, and including "dm-aggregates" overrode our method with its own "count" method.

All of the methods provided by dm-aggregates rely on the adapter defining an "aggregate" method that handles the construction of aggregate queries. Since this method isn't required for a DM adapter, and didn't really make sense for our use, we hadn't defined it.

@mislav mislav closed this Sep 18, 2013

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