Probably also worth moving the count method up onto the User class so that it doesn't clash with the count method defined by Enumerable.

Awesome - thanks so much. I improved the test around #count a little too here #8 - If the test is valid then it seems it's not clashing with Enumerable. What you think?

@darragh Definitely an improvement, but doesn't fix the clash.

What I mean by clash is that Enumerable#count defines additional "modes" of behaviour (e.g. it takes an optional argument and an optional block), so callers may expect those semantics to be preserved. UserCollectionProxy#count will shadow Enumerable#count, so you won't see anything break in the specs, because they aren't testing the full behaviour of Enumerable#count.

Re-implementing that behaviour is a lot of work for little benefit, so easier to move the UserCollectionProxy#count method elsewhere so it doesn't shadow Enumerable#count.

It seems unlikely to us right now that anyone would want to use Enumerable#count with a block/argument on the collection, but that's the point, you never know. Potential for subtle bugs to crop up in the future elsewhere.

Cool. What do you think about this approach?

f177830

To explicitly raise an error?

Also - does it matter if I move the include Enumerable to the top?

No difference if you put the include at the top. Including a module adds it to the module inheritance/ancestor chain/hierarchy, it doesn't actually define any methods on the class that includes it. Raising an error is the same problem as not handling the block at all, it's redefining the signature/semantics of the existing method.

Are there any problems with moving the method to Intercom::User.count other than backwards compatibility?

Yeah - I guess it's just backwards compatibility to move it to Intercom::User.count

So is the problem with count that people will see Intercom::User.all includes Enumerable but get confused as to why it doesn't fully support the Enumerable interface wrt #count? Or are there also subtle problems it will introduce?

Yep exactly. This came about because I tried calling Intercom::User.all.each_with_object, expecting the collection to have included Enumerable. I can imagine a similar wtf moment happening eventually when someone tries to use count with an argument/block.

We could remove UserCollectionProxy#count and let Enumerable#count return the same result, but then it would be making more network calls than necessary when the collection has more than one page. So by introducing Enumerable the UserCollectionProxy#count method essentially becomes an optimisation for the common case.

@darragh I've implemented what I was trying to describe in 557021b and 9cccbed. It's backwards compatible from an API point of view, but not from a network optimisation point of view. So this will still work, it'll just be inefficient:

Intercom::User.all.count

Easy to fix by changing to this:

Intercom::User.count

Thoughts?

So perhaps something like:

def count(item=nil)
  return super(item) unless item.nil? 
  return super(item) if block_given?
  Intercom::User.count
end

I'm not sure if people do rely on all.count - but if they do, it'd be a really bad regression to start loading all users to get that number. I think we can honour the enumerable api and achieve that.

Thanks for following up on this again, and sorry I let it languish.

@darragh Personally I would choose the performance hit because I don't think it's good practice to redefine Enumerable methods, but if you want to maintain the existing behaviour I can't think of an alternative.

Implemented in 2ca5cc5, as per your suggestion. Tagged as :nodoc: because I think it's better to emphasize the use of the new Intercom::User.count method. Can then potentially deprecate/remove the optimised Intercom::UserCollectionProxy#count in a major version bump.

Good idea with :nodoc:, and removing at some later point. Looks great now. Merging!

@darragh Excellent, thanks!

Published new version to rubygems.org: https://rubygems.org/gems/intercom

Thanks again for this great improvement.

On Mon, Mar 4, 2013 at 4:37 PM, Tim Craft notifications@github.com wrote:

@darragh https://github.com/darragh Excellent, thanks!

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/7#issuecomment-14390128
.