Skip to content

adding shortcut: find(x) = find(:one, :from => x) when x is a url #7

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

Closed
wants to merge 1 commit into from

Conversation

sfaxon
Copy link
Contributor

@sfaxon sfaxon commented Apr 11, 2012

No description provided.

@sfaxon
Copy link
Contributor Author

sfaxon commented Apr 11, 2012

I would really appreciate any feedback on this. This is a very early part of an idea to decouple URL generation from ActiveResource. It seems more 'RESTful' to store the URL of a resource rather than an arbitrary ID. Eventually I think it would be useful to use a finder like this when dealing with associations as well. For example:

class Post < ActiveResource::Base
  self.site = "http://localhost:3000"
  has_many :comments
end

class Comment < ActiveResource::Base
  belongs_to :post
end

p = Post.find('http://localhost:3000/posts/1')
GET http://localhost:3000/posts/1 delivers:

{ title: "Post Title",
body: "Awesome post.",
comments_url: "http://localhost:3000/posts/1/comments"
comment_count: 1000 }

Calling p.comments could use Comments.find(p.comments_url), rather than being generated by ActiveResource.

A better example is probably pagination. Github uses the Link header for their pagination. It would be great to see ActiveResource eventually support something like this.

class GitHubRepos < ActiveResoure::Base
  self.site = "https://api.github.com/repos"
  self.paginator = GitHubPaginator.new
end

This probably introduces some problems with parsing paginated results and index actions as well, but it would make ActiveResource a lot more useful for my needs.

thanks

@jeremy
Copy link
Member

jeremy commented Apr 12, 2012

I like this direction, too: working with URLs and Link headers. Navigating an API.

@SweeD
Copy link
Collaborator

SweeD commented Apr 15, 2012

I like the idea, too. +1 from me.

@SweeD
Copy link
Collaborator

SweeD commented Apr 18, 2012

Some more feedback or just merge?

/cc @arunagw @guilleiguaran

@arunagw
Copy link
Member

arunagw commented Apr 18, 2012

@SweeD thanks for mention. 👍 from my side.

@jeremy
Copy link
Member

jeremy commented Apr 18, 2012

The implementation feels bolted on. I think working with URIs natively needs deeper consideration.

Parsing every id as a URI will be surprising in many cases, too: Person.find(params[:id]) is now vulnerable to someone passing http://badsite.com/fakeperson/1 as the id param.

@sfaxon
Copy link
Contributor Author

sfaxon commented Apr 18, 2012

I agree that it feels bolted on. Currently the find method works the way it did in ActiveRecord, using the first param to find out what sort of search it should be doing. Is there a better way do find overall?

@jeremy
Copy link
Member

jeremy commented Apr 18, 2012

Checking for a URI instance could work, rather than accepting strings and parsing.

@guilleiguaran
Copy link
Member

I would love

GitHubRepos.find(params[:id], :site => "https://api.github.com/repos")

@SweeD
Copy link
Collaborator

SweeD commented Apr 24, 2012

@guilleiguaran

There is already a option for that:

GitHubRepos.find(params[:id], from: "https://api.github.com/repos")

Or did you mean especially the site attribute or haven't i got your question at all? 😄

@SweeD
Copy link
Collaborator

SweeD commented Apr 24, 2012

@jeremy said:

Checking for a URI instance could work, rather than accepting strings and parsing.

Sounds like a goog idea to me.

What do you guys think?
/cc @guilleiguaran @arunagw

@guilleiguaran
Copy link
Member

@SweeD right, I mean the site 😄

@jeremy
Copy link
Member

jeremy commented Oct 19, 2012

@sfaxon thoughts on checking scope.is_a?(URI) instead?

@SweeD
Copy link
Collaborator

SweeD commented Jan 29, 2014

@sfaxon What do you think about jeremy's idea?

@SweeD
Copy link
Collaborator

SweeD commented Nov 3, 2014

@sfaxon Anything new here? 😏

@sfaxon
Copy link
Contributor Author

sfaxon commented Nov 3, 2014

I think the find method should work given a URI, but I think it would also result in some bulky method calls like this:

Post.find(URI.parse('http://localhost:3000/posts/1'))

Which can be easily fixed with a helper method that takes a string, converts to a URI and then calls find.

I'm glad people like the general direction, that's all the feedback I was trying to get. Actually implementing this should probably touch a lot more places to make sure it's consistent, so I'll close the pull request.

Thanks for the feedback!

@sfaxon sfaxon closed this Nov 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants