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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

simonoff
Copy link

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.

@apotonick
Copy link
Member

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

@simonoff
Copy link
Author

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

@simonoff
Copy link
Author

@apotonick any progress?

@apotonick
Copy link
Member

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! ❤️

@@ -17,11 +17,12 @@ class ParseStrategySyncTest < BaseTest

representer!(:module => mod, :name => :song_representer) do
property :title
self.representation_wrap = :song if format == :xml
Copy link
Member

Choose a reason for hiding this comment

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

This changes the test, can you please revert that? In this test I want to make sure representation_wrap works and not :as (at least that's what I remember). 😉

@apotonick
Copy link
Member

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?

@simonoff
Copy link
Author

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

@apotonick
Copy link
Member

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

@simonoff
Copy link
Author

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

@simonoff
Copy link
Author

@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.

@apotonick
Copy link
Member

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

@simonoff
Copy link
Author

@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

@apotonick
Copy link
Member

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.

@simonoff
Copy link
Author

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

@apotonick
Copy link
Member

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.

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.

2 participants