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 register keyword for adding customized parsers and formatters #1330

Merged

Conversation

namusyaka
Copy link
Contributor

ref 93e6998 #1329
I'd like to use grape-msgpack without tricky hacking. Currently, our implementation does not support inheritable interface, so I fixed a few points.

Thoughts?

@namusyaka namusyaka force-pushed the fix-builtin-parsers-and-formatters branch from 2534984 to 7fdded0 Compare March 17, 2016 06:45
@dblock
Copy link
Member

dblock commented Mar 17, 2016

I see how a require fixes this, but we now load formatters we don't use. Why doesn't autoload work with grape-msgpack?

@namusyaka
Copy link
Contributor Author

Nope, I removed autoloads to declare FORMATTERS and PARSERS constants.

@dblock
Copy link
Member

dblock commented Mar 17, 2016

Sorry, I am a little slow, and clearly I am missing some Ruby-fu understanding :)

Why doesn't it work with grape-msgpack?

@namusyaka
Copy link
Contributor Author

At first, grape-msgpack didn't consider the commit.
However, something like this line has broke inheritance interface.
Our provided options cannot be inherited, so if we want to implement customized grape formatter, we must require the tricky code.

I think we should be able to provide more developer-friendly interface.
If this PR is merged, maybe grape-msgpack's can be resolved by a little bit changes.

@dblock
Copy link
Member

dblock commented Mar 17, 2016

What if we just added a global interface to register formatters, instead of inheriting anything? Like so:

Grape.configure do |config|
   config.formatters.add
end

@dblock
Copy link
Member

dblock commented Mar 17, 2016

Or

class MyFormatter << Grape::Formatter
end

Grape::Formatter.register MyFormatter, ...

@namusyaka
Copy link
Contributor Author

Yeah, it's so cool and developer-friendly. :)
Btw, should we contain the configuration feature in 0.16.0?
Also, can this PR be used as a 0.15-backported patch?

2016年3月17日木曜日、Daniel Doubrovkine (dB.) @dblockdotorg<
notifications@github.com>さんは書きました:

What if we just added a global interface to register formatters, instead
of inheriting anything? Like so:

Grape.configure do |config|
config.formatters.addend


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#1330 (comment)

@namusyaka
Copy link
Contributor Author

Hmm, right. It's reasonable.

2016年3月17日木曜日、Daniel Doubrovkine (dB.) @dblockdotorg<
notifications@github.com
javascript:_e(%7B%7D,'cvml','notifications@github.com');>さんは書きました:

Or

class MyFormatter << Grape::Formatterend
Grape::Formatter.register MyFormatter, ...


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#1330 (comment)

@dblock
Copy link
Member

dblock commented Mar 17, 2016

I don't think we should backport anything and just move forward. If you want to give one of the above a short, go for it! I'd rather not remove autoload. I prefer the second version btw, not the global configuration one because that always bites us.

@Vsandhya11
Copy link

I am planning to write micro service using ruby-grape, So I am thinking 0.15.0 is stable version to use. Plz lemme know your suggestions.

@namusyaka namusyaka force-pushed the fix-builtin-parsers-and-formatters branch from 7fdded0 to f96b732 Compare March 18, 2016 01:36
@namusyaka
Copy link
Contributor Author

I've tried to implement register keyword on those by using the second feature as base approach.
@dblock Could you confirm the changes?

@@ -11,6 +11,7 @@

* [#1325](https://github.com/ruby-grape/grape/pull/1325): Params: Fix coerce_with helper with Array types - [@ngonzalez](https://github.com/ngonzalez).
* [#1326](https://github.com/ruby-grape/grape/pull/1326): Fix wrong behavior for OPTIONS and HEAD requests with catch-all - [@ekampp](https://github.com/ekampp), [@namusyaka](https://github.com/namusyaka).
* [#1330](https://github.com/ruby-grape/grape/pull/1330): Fix built-in parsers and formatters - [@namusyaka](https://github.com/namusyaka).
Copy link
Member

Choose a reason for hiding this comment

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

It's not clear what's being fixed. The built in parsers and formatters weren't broken really.

@dblock
Copy link
Member

dblock commented Mar 18, 2016

I'll merge this as is, feel free to make further changes later. Also I think we don't need to document this publicly for now.

dblock added a commit that referenced this pull request Mar 18, 2016
…tters

Fix built-in parsers and formatters
@dblock dblock merged commit 44418cc into ruby-grape:master Mar 18, 2016
@dblock
Copy link
Member

dblock commented Mar 18, 2016

Add a similar spec for parsers.

@namusyaka namusyaka deleted the fix-builtin-parsers-and-formatters branch March 18, 2016 11:41
namusyaka added a commit to namusyaka/grape that referenced this pull request Mar 18, 2016
dblock added a commit that referenced this pull request Mar 18, 2016
@namusyaka namusyaka changed the title Fix built-in parsers and formatters Add register keyword for adding customized parsers and formatters Mar 18, 2016
przbadu pushed a commit to przbadu/grape that referenced this pull request Apr 5, 2016
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