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

Change to_xml method options in the default error_formatter #290

Merged
merged 8 commits into from
Dec 13, 2012

Conversation

dpsk
Copy link
Contributor

@dpsk dpsk commented Dec 13, 2012

I think it's better to have something like this

  <?xml version="1.0" encoding="UTF-8"?>
  <error>
    <message>Contact not found</message>
  </error>

instead of:

  <?xml version="1.0" encoding="UTF-8"?>
  <hash>
    <error>Contact not found</error>
  </hash>

hash as root looks a little off.

@dblock
Copy link
Member

dblock commented Dec 13, 2012

I think it makes sense.

  • Please add tests. Looks like we didn't have any tests for the xml output, bad on us.
  • Add an entry in CHANGELOG.
  • Possibly add something on XML format to README - it's not much used, so it's good to see people who actually use it make it better and document it properly.

@dpsk
Copy link
Contributor Author

dpsk commented Dec 13, 2012

Okay, i will add these changes a bit later.

@dpsk
Copy link
Contributor Author

dpsk commented Dec 13, 2012

I'm added specs and changelog entry. I didn't have anything to add to the documentation to be honest.
As for the specs, inside of the formatter to_s method is used, instead of to_xml.
For using to_xml methods in specs we need to require active_support/core_ext and add builder.
https://github.com/lifo/docrails/blob/master/activesupport/lib/active_support/builder.rb

Oh, to_s on hash instance behaves differently in 1.8 and 1.9 :(

end
get '/'
last_response.body.should == {:message=>"rain!"}.to_s
end
Copy link
Member

Choose a reason for hiding this comment

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

I am confused. How is this XML?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The object don't respond to the to_xml method, what you see is the output of to_s. For testing to_xml we need to add active_support and builder(https://github.com/lifo/docrails/blob/master/activesupport/lib/active_support/builder.rb) into dependencies, the question is should we?

Copy link
Member

Choose a reason for hiding this comment

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

Well, if we want to test XML, we definitely should!

@dpsk
Copy link
Contributor Author

dpsk commented Dec 13, 2012

Sorry for al those messy commits. Now we have proper xml in the test and successfull build.

@dblock
Copy link
Member

dblock commented Dec 13, 2012

This is good, thanks, merging.

dblock added a commit that referenced this pull request Dec 13, 2012
Change to_xml method options in the default error_formatter
@dblock dblock merged commit 70809e2 into ruby-grape:master Dec 13, 2012
@dblock dblock mentioned this pull request Dec 14, 2012
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 7984e1f on dpsk:master into * on intridea:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 7984e1f on dpsk:master into * on intridea:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 7984e1f on dpsk:master into * on intridea:master*.

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.

3 participants