Skip to content

Conversation

@lsylvester
Copy link
Contributor

Adds option to has_many relationships have links (links: true) and turn of data (data: false) for jsonapi.

Replaces #840
Fixes #834

@joaomdmoura
Copy link
Member

Hey @lsylvester, I want to review this one, see cool! Could you rebase it please? 😄

@lsylvester lsylvester force-pushed the jsonapi-link-v-data branch from a3ffb42 to 5d5715a Compare August 10, 2015 00:18
@lsylvester
Copy link
Contributor Author

@joaomdmoura Rebased.

@kimsuelim
Copy link

@lsylvester, How does this work for namespace url?

For example:
/api/posts/1/comments, api_post_comments(post)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hiya, Thanks for this. Would you mind helping me understand these changes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, could you help us on understanding these? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to_param and to_model are used in the for url generation.

to_model is called by polymorphic_url and it is expected to return an ActionModel compliant model. Without it, the polymororphic_urlv method will error.
to_param is used to generate the value for the object in the url. Without it, I get a url like http://test.host/tags/%23%3CTag:0xXXXXXX%3E/posts\

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Nice, could you add some comments on top of those method explaining this to help new contributors when jumping into the code 😄

@bf4
Copy link
Member

bf4 commented Aug 20, 2015

@lsylvester You still working on this?

@lsylvester lsylvester force-pushed the jsonapi-link-v-data branch from 5d5715a to 3cbd327 Compare August 25, 2015 05:49
@lsylvester
Copy link
Contributor Author

@kimsuelim The is currently no way to provide a name spaced url. Should this be a config option like

ActiveModel::Serializer.config.url_namespace = :api

or

class SomeSerializer < ActiveModel::Serializer
  url_namespace :api
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might this PR want to talk to #1018 (though these are different links)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1018 is kind stopped for sometime, I wouldn't worry abou that right now, but indeed:

So the link building code should ideally be reusable in different contexts

This would be nice.

@joaomdmoura
Copy link
Member

This is really nice @lsylvester great work, I'm looking forward to see this merged.
I made some small comments other the that, this PR looks good.

I would like to ask to also add some docs about this on our new docs, maybe a new article on how to section.
btw I know it also represents some more dependencies on Rails and that's the way things are going, but would be nice to have on the docs some work arounds ppl using AMS on PORO could do in order to be able to make it work.

Sorry about this again 😁, could you rebase it?

@tchak
Copy link
Contributor

tchak commented Oct 20, 2015

@lsylvester are you still actively working on this?

It would be also nice to be able to pass an arbitrary object with links

@tchak
Copy link
Contributor

tchak commented Oct 20, 2015

I rebased this PR in #1282 and added more tests

@lsylvester
Copy link
Contributor Author

@tchak I am happy for you to take over this one. Closing in favour of #1282

@lsylvester lsylvester closed this Oct 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants