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

Don't require multi_json and multi_xml. #1623

Merged
merged 3 commits into from Jun 14, 2017

Conversation

Projects
None yet
2 participants
@dblock
Member

dblock commented May 2, 2017

I think we need to add explicit configuration to this and tests that use different json and xml libraries.

@dblock

This comment has been minimized.

Show comment
Hide comment
@dblock

dblock May 2, 2017

Member

@namusyaka How do we feel about this?

Member

dblock commented May 2, 2017

@namusyaka How do we feel about this?

@dblock dblock requested a review from namusyaka May 2, 2017

Json = ::JSON
Json::ParseError = Json::ParserError
end
end

This comment has been minimized.

@namusyaka

namusyaka Jun 9, 2017

Member

I'd like to declare require 'json' explicitly instead of trying to require it here.

@namusyaka

namusyaka Jun 9, 2017

Member

I'd like to declare require 'json' explicitly instead of trying to require it here.

This comment has been minimized.

@dblock

dblock Jun 12, 2017

Member

What do you mean exactly? That the user has to require 'multi_json' explicitly instead of trying to require and catch LoadError here? That's a bit tricky because that forces you to include multi_json before Grape, doesn't it?

@dblock

dblock Jun 12, 2017

Member

What do you mean exactly? That the user has to require 'multi_json' explicitly instead of trying to require and catch LoadError here? That's a bit tricky because that forces you to include multi_json before Grape, doesn't it?

else
Xml = ::ActiveSupport::XmlMini
Xml::ParseError = StandardError
end

This comment has been minimized.

@namusyaka

namusyaka Jun 9, 2017

Member

Likewise, I'd like to declare require 'active_support/xml_mini' explicitly.
At the moment we don't declare require 'active_support' itself, so autoload won't be worked.

@namusyaka

namusyaka Jun 9, 2017

Member

Likewise, I'd like to declare require 'active_support/xml_mini' explicitly.
At the moment we don't declare require 'active_support' itself, so autoload won't be worked.

This comment has been minimized.

@dblock

dblock Jun 12, 2017

Member

Or are you saying that I should add require 'active_support/xml_mini' here after else?

@dblock

dblock Jun 12, 2017

Member

Or are you saying that I should add require 'active_support/xml_mini' here after else?

This comment has been minimized.

@namusyaka

namusyaka Jun 14, 2017

Member

Yes. Do I understand things correctly?

@namusyaka

namusyaka Jun 14, 2017

Member

Yes. Do I understand things correctly?

This comment has been minimized.

@dblock

dblock Jun 14, 2017

Member

Ok, take a look at the PR as now, any issues? Thank you.

@dblock

dblock Jun 14, 2017

Member

Ok, take a look at the PR as now, any issues? Thank you.

@namusyaka

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Jun 9, 2017

Member

Sorry for the delay. I strongly agree with the purpose of this pull request.
Thank you!

Member

namusyaka commented Jun 9, 2017

Sorry for the delay. I strongly agree with the purpose of this pull request.
Thank you!

@dblock dblock changed the title from [WIP] Don't require multi_json and multi_xml. to Don't require multi_json and multi_xml. Jun 13, 2017

@dblock

This comment has been minimized.

Show comment
Hide comment
@dblock

dblock Jun 13, 2017

Member

@namusyaka I made the change that requires you to require multi_* manually. If you care to take a look? I'm going to merge soon.

Member

dblock commented Jun 13, 2017

@namusyaka I made the change that requires you to require multi_* manually. If you care to take a look? I'm going to merge soon.

@namusyaka

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Jun 14, 2017

Member

👍 👍 👍

Member

namusyaka commented Jun 14, 2017

👍 👍 👍

@dblock dblock merged commit 53aeee9 into ruby-grape:master Jun 14, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@namusyaka

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Jun 14, 2017

Member

Thank you for your great work! 🎉

Member

namusyaka commented Jun 14, 2017

Thank you for your great work! 🎉

@dblock dblock deleted the dblock:mutli-json-xml branch Jul 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment