representable icon indicating copy to clipboard operation
representable copied to clipboard

XML changes: Avoid to set tag name from representer. Use as option instead.

Open simonoff opened this issue 10 years ago • 14 comments

I have a case when I need to use same decorator for different properties. Just now I using subclasses for it what is annoying when you have a lot of same attribute types. With this PR I will be able to use as option for each property.

simonoff avatar Jun 11 '15 14:06 simonoff

Can you make an example how this helps, I didn't really understand it by looking at the tests. Thanks, man!

apotonick avatar Jun 11 '15 22:06 apotonick

@apotonick I added test case with example. So I think you will get the point.

simonoff avatar Jun 12 '15 07:06 simonoff

@apotonick any progress?

simonoff avatar Jun 15 '15 08:06 simonoff

Sorry to disappoint you, Alex, but XML support in Representable has a really low priority right now in my life - I promise you to look over it this week! :heart:

apotonick avatar Jun 15 '15 09:06 apotonick

This PR adds the :as option so the wrap for nested representers can be specified from the composing representer. It doesn't look at the representation_wrap if :as is provided.

Is that correct?

apotonick avatar Jun 16 '15 02:06 apotonick

Yes, but as I understand :as provided all time.

simonoff avatar Jun 16 '15 07:06 simonoff

Yeah, I remember :as was there, before. But this didn't work with nested representers??

apotonick avatar Jun 16 '15 22:06 apotonick

@apotonick no. And inside :as option all time present. Even if you not set it. I suppose it taken from class/module name.

simonoff avatar Jun 17 '15 14:06 simonoff

@apotonick another thing. It doesn't look at the representation_wrap if :as is provided. is not true. It replace tag name with option what comes in :as. So no need in representation_wrap option.

simonoff avatar Jun 17 '15 14:06 simonoff

I sloooooowly start to remember this problem! It's great you came up with it! :+1:

apotonick avatar Jun 17 '15 22:06 apotonick

@apotonick So I fixed it property? I think not fully because still not possible to set wrapper tag for collections, but I think it could be possible through something like this:

nested :songs do
  collection :songs, as: :song, decorator: ::SongDecorator
end

simonoff avatar Jun 18 '15 07:06 simonoff

I am working on fixing this - it's a terrible mess in the current implementation and I might have to break the API.

So, to sum up what you need.

class InvoiceDecorator < Representable::Decorator
  include XML

  property :start_date, wrap: :start_date, decorator: DateDecorator
end

Where the path would end up as invoice/start_date and not as currently, invoice/start_date/start_date.

apotonick avatar Jul 13 '15 05:07 apotonick

@apotonick exactly! I think what my fix is not help you.

simonoff avatar Jul 13 '15 07:07 simonoff

I think there's two problems in the current XML implementation.

  1. As you said, the as: option doesn't work - I think this would fix your problem.
  2. We support a weird :wrap option that could easily be achieved with ::nested. In JSON/Hash, I introduced a :wrap option that allows to suppress the representation_wrap of an inline representer, and that should be the same in XML.

apotonick avatar Jul 13 '15 08:07 apotonick