Skip to content
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

add Representable::JSON#collection_wrapper #22

Closed
wants to merge 1 commit into from
Closed

add Representable::JSON#collection_wrapper #22

wants to merge 1 commit into from

Conversation

timoschilling
Copy link

this is a alternative way compare to #20 to make Collection Representers more DRY

this simplify the following:

module SongRepresenter
  include Representable::JSON

  property :title
end

module SongsRepresenter
  include Representable::JSON::Collection

  items :class => Song
end

[Song.new(:title => "Fallout"), Song.new(:title => "Synchronicity")].extend(SongsRepresenter).to_json

to this:

module SongRepresenter
  include Representable::JSON
  property :title
end

[Song.new(:title => "Fallout"), Song.new(:title => "Synchronicity")].extend(SongRepresenter.collection_wrapper(:class => Song)).to_json

Im not really sure what is the best method name is, #collection_wrapper or #collection_representer?

The same should be follow as XML::Collection when #19 works

@iamjwc
Copy link

iamjwc commented May 21, 2013

Any reason why this hasn't been pulled in or rejected in 7 months? I'm wondering how to do the same thing and this seems good to me.

Thanks,

Justin

@apotonick
Copy link
Member

I like the idea, working on it.

@oliverbarnes
Copy link

Looking to use this too, with Grape. Would love to simply do

[Song.new(:title => "Fallout"), Song.new(:title => "Synchronicity")].extend(SongRepresenter).to_json

Implicitly detecting when it's a collection extending the representer, and inferring the class of the resource . Is that what you intended @timoschilling ?

@apotonick
Copy link
Member

Yes and no (I am not @timoschilling). What you suggest @oliverbarnes is implicit (aka "magic") mechanics, whereas Timo's example is explicit.

[Song.new(:title => "Fallout"), Song.new(:title => "Synchronicity")].
  extend(SongRepresenter.collection_wrapper(:class => Song)).to_json

Here, you extend the array with a representer. I favour the second, explicit way as we're not gonna monkey-patch Array with representable rubbish 😄 Would that help, @oliverbarnes ???

@oliverbarnes
Copy link

Yes, I mean magic but just internalizing what collection_wrapper does. Naive late-night pseudo-code:

module Representable
  module JSON
    def self.included(base)
      base.class_eval do
        ...
        if base.is_a? Array
          include Representable::Hash::Collection
          items( extend: self, class: extract_from_self_class_name )
        end
      end
    end

(corrected after seeing Representable::JSON::Collection also includes Representable::JSON)

@apotonick
Copy link
Member

Interesting idea- so your call would look like

[Song, Song].extend(SongRepresenter).to_json #=> "[{title: ...]

@oliverbarnes
Copy link

Yeah ;) but does it work? Need to take a look at it with fresh eyes tomorrow. I can test it here and send another PR if it does

@apotonick
Copy link
Member

It does not work, yet, and I wouldn't add that per default (too much magic) but we could provide a module to activate that behaviour.

module SongRepresenter
  include Representable::JSON
  include Representable::JSON::CollectionRepresenter # or AutoCollection, or whatever

  # define singular representer here
  property :title
end

This would allow using the representer both as a singular and for collections, just as Timo suggested. I'm a bit excited!

@oliverbarnes
Copy link

I like it! Nice and declarative. I agree, this is better - the magic could come back to bite unexpectedly. Let me know if I can help in any way

@dtshepherd
Copy link

I like it too, it would cut out my custom generators I created to get
around this problem :)

On Thu, Apr 24, 2014 at 8:22 PM, Oliver Azevedo Barnes <
notifications@github.com> wrote:

I like it! Nice and declarative. I agree, this is better - the magic could
come back to bite unexpectedly. Let me know if I can help in any way


Reply to this email directly or view it on GitHubhttps://github.com//pull/22#issuecomment-41348085
.

@apotonick
Copy link
Member

The thing here is, once you have this automatic collection representer creation, people are gonna come up with requests and configuration options to fine tune it - which is contradicting to my attempt to educate people to use more explicit code (e.g. write a separate collection representer module instead of if/else, config options in method calls, etc)...

I'm totally fine with a basic creator, though.

@oliverbarnes
Copy link

I see your point. While writing this pseudo-code I was already thinking about how to make it flexible, then restrained myself to a smart default people should take or leave. Making it an optional and deliberate declaration makes it clear people should just create their own collection representers if their use cases are different than this.

@apotonick
Copy link
Member

I would like to implement both ideas.

@timoschilling (explicit)

[Song.new(:title => "Fallout"), Song.new(:title => "Synchronicity")].extend(SongRepresenter.collection_wrapper(:class => Song)).to_json

@oliverbarnes (implicit, with a bit of explicitness)

[Song.new(:title => "Fallout"), Song.new(:title => "Synchronicity")].extend(SongRepresenter).to_json

..where you have to allow the creation of a collection representer from SongRepresenter.

module SongRepresenter
  include Representable::JSON
  include Representable::JSON::CollectionRepresenter # or AutoCollection, or whatever

  # define singular representer here
  property :title
end

apotonick added a commit to trailblazer/roar that referenced this pull request Jul 20, 2014
@apotonick
Copy link
Member

Thanks for your great ideas, @oliverbarnes and especially @timoschilling ❤️ The explicit collection for modules (for Decorator coming in a few hours) already works in master. The implicit generation is coming soon, too!

@apotonick apotonick closed this Jul 21, 2014
@apotonick
Copy link
Member

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.

None yet

5 participants