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

Support Singleton Routes #4

Merged
merged 1 commit into from Apr 4, 2012

Conversation

Projects
None yet
3 participants
Contributor

kenmazaika commented Mar 25, 2012

This is a version of this pull request, for the ActiveResource repo instead of the Rails repo:

rails/rails#5361

I was interfacing with a third party who exposed an API with a RESTful singleton route. In order to support this with ActiveResource I made this patch.

Initially I planned to patch ActiveResource::Base to support this, but very quickly it became convoluted with conditionals to determine which mode the model was in (singleton or standard). It was much easier to make a subclass, and have clients inherit from this.

I tried to be consistent with ActiveResource::Base. The one exception that is not consistent is find. ActiveResource::Base has the following code:

  def find(*arguments)
    scope   = arguments.slice!(0)
    options = arguments.slice!(0) || {}

In the singleton case however, find does not have a concept of a scope (or rather, the scope is always :singleton). Instead of forcing users to type an option that is not configurable, I thought it made more sense to use a sensible default, and alter how the method processes arguments.

To test the change, I made this simple singleton route API with an associated ActiveResource implementation:

https://github.com/kenmazaika/weather

If this is a feature that is not in ActiveResource for a reason, feel free to close this ticket out and not merge it. I did find some other people looking for this functionality though:

https://rails.lighthouseapp.com/projects/8994/tickets/760-activeresource-with-map-resource
https://rails.lighthouseapp.com/projects/8994/tickets/4348-supporting-singleton-resources-in-activeresource
http://railsforum.com/viewtopic.php?id=23172

If there are changes that need to happen to get this merged in, please let me know. I'd be happy to make them.

Owner

jeremy commented Mar 26, 2012

Thanks for digging in on this, @kenmazaika.

I usually work around this with something like

class Account < ActiveResource::Base
  def self.find_singleton
    find :one, from: '/account.xml'
  end
end

Native support would be pretty nice, and subclassing sounds reasonable. Could just do class ActiveResource::Singleton < ActiveResource::Base instead of introducing another Base.

Contributor

kenmazaika commented Mar 26, 2012

I would be happy to make the change from

ActiveResource::Singleton::Base

to

ActiveResource::Singleton

Are there any other changes that I should make, while I'm at it?

Owner

jeremy commented Mar 26, 2012

@kenmazaika looking at my own code, it'd be hard to use this new subclass since I already rely on an abstract base class: class Basecamp::Base < ActiveResource::Base.

This is a pretty common pattern. Subclassing Singleton throws in a monkey wrench. Perhaps it'd be better served as a Singleton mixin.

Collaborator

SweeD commented Mar 27, 2012

I like the idea with the mixin the best.

What about ActiveResource::Singleton?

Contributor

kenmazaika commented Mar 27, 2012

I'd be happy to change this to a mixin.

So that include ActiveResource::Singleton would make anything that inherits from ActiveResource::Base to act like ActiveResource::Singleton::Base in my original pull request.

I'll probably get around to making this change sometime next weekend. I'll add to this pull request, and make a comment to ping you guys to look at it.

If there are any other changes that should happen, let me know. Thanks.

Owner

jeremy commented Mar 27, 2012

Great! Thanks @kenmazaika

Contributor

kenmazaika commented Apr 1, 2012

@jeremy @SweeD

I updated the pull request to use a mixin rather than a new sublcass. Let me know if you see anything else that should be changed with this pull request. Thanks.

Collaborator

SweeD commented Apr 1, 2012

Good job @kenmazaika.

Looks good to me and tests are green, too.

Could you please squash these 2 commits into 1 clean commit?

Contributor

kenmazaika commented Apr 1, 2012

@SweeD done.

Owner

jeremy commented Apr 3, 2012

Looks good @kenmazaika! 👍 for merge when that's fixed, @SweeD

Add support for Singleton Routes.
Implement ActiveResource::Singleton as a mixin, instead of using ActiveResource::Singleton::Base as a subclass.
Contributor

kenmazaika commented Apr 4, 2012

@SweeD @jeremy done. Also squashed into previous commit.

Collaborator

SweeD commented Apr 4, 2012

@kenmazaika Good job, thank you! :)

SweeD added a commit that referenced this pull request Apr 4, 2012

@SweeD SweeD merged commit fd79797 into rails:master Apr 4, 2012

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