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 basic support for xml #1133

Closed
wants to merge 1 commit into from
Closed

Conversation

beauby
Copy link
Contributor

@beauby beauby commented Sep 10, 2015

It's all in the title: do render xml: resource instead of render json: resource, and you get xml (the actual format will depend on the adapter used).

@bf4
Copy link
Member

bf4 commented Sep 10, 2015

Looks fine, missing as_xml and delegation to adapter in serializable resource, but i hope we're not promissong too mich by adding it... All rails models already support xml...

I have some thougts on design for supporting sifferent mime types
Also.See my serializers pr in Rails

Who wants to maintain it in ams?

@bf4
Copy link
Member

bf4 commented Sep 10, 2015

Also, AMS has json code all over..

@beauby
Copy link
Contributor Author

beauby commented Sep 10, 2015

@bf4 Does it? I don't see much code in AMS that is really json specific.

def test_render_object_xml
get :render_object_xml

hash = Hash.from_xml(@response.body).each_value.first
Copy link
Member

Choose a reason for hiding this comment

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

@bf4
Copy link
Member

bf4 commented Sep 11, 2015

@beauby What do you think of, instead of adding this code to AMS, writing it up in docs, maybe still test it?

e.g.

# docs/other_mime_type.md

See the Rails built-in xml Renderer and how to override it
- https://github.com/rails/rails/blob/4-2-stable/actionpack/lib/action_controller/metal/renderers.rb#L66-L128

Or

Just override the xml renderer method Rails defines

# docs/xml_mime_example.rb
# In your ApplicationController or a mixin,
# Rails 4.0 - 4.1, use `:_render_option_xml`
# Rails 4.2 use `:_render_with_renderer_xml`
# Rails 5 TBD


def _render_with_renderer_xml(xml, options)
  options[:adapter] ||= :flatten_json # or :attributes, hopefully
  serializable_resource = get_serializer(xml, options)
  if serializable_resource == xml
    super
  else
    super(serializable_resource, options).serializable_hash(options).to_xml
  end
end

@beauby
Copy link
Contributor Author

beauby commented Sep 11, 2015

@bf4 I do not have any strong feeling either way, to be honest. I made the PR because it seemed like a "quick win" for the project, but I personally have no use for it, so I'm fine if we decide to make it a note instead!

@bf4
Copy link
Member

bf4 commented Oct 2, 2015

Can we close this?

@NullVoxPopuli
Copy link
Contributor

I think so

@NullVoxPopuli
Copy link
Contributor

Unless we actually want to support xml. IMO, it needs to die. (Xml)

@beauby
Copy link
Contributor Author

beauby commented Oct 2, 2015

I was keeping this issue open until we make a line in the docs about it.

Le vendredi 2 octobre 2015, L. Preston Sego III [email protected]
a écrit :

Unless we actually want to support xml. IMO, it needs to die. (Xml)


Reply to this email directly or view it on GitHub
#1133 (comment)
.

Lucas Hosseini
[email protected]

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.

3 participants