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

Disabled formatting via and added support for :serializable_hash in API settings. #282

Merged
merged 6 commits into from
Dec 1, 2012

Conversation

dblock
Copy link
Member

@dblock dblock commented Nov 30, 2012

Refactored formatters and parsers. This also resolves #273. Users that want to explicitly use serializable_hash can via a serializable_hash format.

@dblock
Copy link
Member Author

dblock commented Nov 30, 2012

The Travis failure is a fluke, https://travis-ci.org/intridea/grape/builds/3440430.

@dnd
Copy link
Contributor

dnd commented Dec 1, 2012

It looks pretty good. My only question/comment would be why are the normal and error formatters separate? Why not have the formatter implement #encode and #encode_error or something like that.

I see that as maybe being useful if I define a custom formatter. Say I define an XlsFormatter right now, how do I specify the error formatter for that?

@dblock
Copy link
Member Author

dblock commented Dec 1, 2012

Thanks @dnd. The error formatter formats errors that are messages, backtraces, etc., the output formatter just formats raw data. So they accomplish similar but different things, even if they have a similar structure.

Do you think it's actually a useful feature to define an error formatter based on content type?

dblock added a commit that referenced this pull request Dec 1, 2012
Disabled formatting via  and added support for :serializable_hash in API settings.
@dblock dblock merged commit 26ee18f into ruby-grape:master Dec 1, 2012
@dnd
Copy link
Contributor

dnd commented Dec 2, 2012

I would think it could be useful. No, maybe there isn't a good error output for an Excel spreadsheet, but maybe I'm using a specialized json format, or just some other type of content that could have a usefully presented error message in said format(YAML for instance).

We can't possibly conceive all of the formats people would want, so why not just provide an easy way for them to implement both aspects.

@dblock
Copy link
Member Author

dblock commented Dec 2, 2012

With #284 you can write your own formatter:

error_format :custom
error_formatter :custom, CustomFormatter

The next question is how does that formatter get selected? In the code above it will always use the custom error formatter. The syntax is a bit weird, since something like this is possible, but completely useless:

error_format :json
error_formater :custom, CustomFormatter # this formatter will never get used

Should we make the error format selection based on content-type that has been selected for the request data? In which case we can probably remove the error_format directive altogether.

Thoughts?

@dblock
Copy link
Member Author

dblock commented Dec 2, 2012

On your earlier comment on why do we not want a formatter implement all of parse, encode and encode_error, I don't know. I feel that these formatters/parsers are best separate, but I can also see how they could be the same one. For the user this would mean having to write all 3 functions to override a single formatter's behavior (eg. the JSON error formatting), but then again you could inherit from the existing formatter if you wanted to.

@dblock
Copy link
Member Author

dblock commented Dec 2, 2012

I wrote #285 for the first part. I like it, it removes a ton of confusion around these custom error formats, but I will leave it open for now to get feedback.

I also tried to do the second part of merging formatters, but there're some difficulties. The first one is that selection of parsers vs. error formatter is different (the parser needs to return nil when a parser is not found, the formatter returns the object being formatted as is). The second is that we let you do error_formatter :json, lambda { ... }, meaning we still need to keep collections of formatters separately - here we will need a different formatter for errors and output.

@dnd
Copy link
Contributor

dnd commented Dec 2, 2012

Yes, they could just inherit from one of the existing classes. I think separate classes/modules for parse and encode/encode_error would still be good as parse deals with input, where the encoding two are for output.

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