add custom error formatter #76

xn · 2017-08-30T19:45:13Z

Would y'all be interested in having an error formatter that would allow for a jsonapi response?

If so, I can add some tests and documentation to this.

dblock · 2017-08-31T01:00:27Z

I think the answer is yes, we want a custom formatter for error messages, please.

xn · 2017-08-31T15:22:51Z

Kk, leaving later today to go camping. I'll finish this out Monday or so when I get back.

xn · 2017-10-02T15:55:44Z

@dblock Rubocop is going to complain about the method signature of #call. I have no control over that. What should I do?

xn · 2017-10-02T18:45:11Z

I don't know what you want to do about 0.8. Please advise.

dblock · 2017-10-02T21:00:12Z

For rubocop run rubocop -a ; rubocop --auto-gen-config.

dblock · 2017-10-02T21:00:27Z

CHANGELOG.md

@@ -2,7 +2,7 @@

 ### 1.5.2 (Next)

-* Your contribution here.


Put this back.

dblock · 2017-10-02T21:01:38Z

grape-active_model_serializers.gemspec

@@ -18,6 +18,7 @@ Gem::Specification.new do |gem|

  gem.add_dependency 'grape', '>= 0.8.0'
  gem.add_dependency 'active_model_serializers', '>= 0.10.0'
+  gem.add_dependency 'multi_json', '~> 1.0'


I don't think we want this. Grape has a way to format JSON regardless of the library used, you can call it with ::Grape::Json.dump.

uninitialized constant Grape::Json

Then do you want to change the version of grape depended on?

Or should we implement the same thing?

This gem depends on grape, so you should be able to use it, I am not sure what the problem is, but maybe you need to make sure to depend on Grape >= 1.0 where this was added.

So, this one should still be removed, it's unnecessary in all Grape versions since it's either coming via Grape or is not used..

dblock · 2017-10-02T21:02:00Z

lib/grape-active_model_serializers/error_formatter.rb

+      extend Base
+
+      class << self
+        # rubocop:disable Metrics/CyclomaticComplexity


Get rid of of the rubocop stuff here and do rubocop -a ; rubocop --auto-gen-config.

Inspecting 31 files ...C........................... Offenses: lib/grape-active_model_serializers/error_formatter.rb:8:9: C: Cyclomatic complexity for call is too high. [7/6] def call(message, backtrace, options = {}, env = nil, original_exception = nil) ^^^ lib/grape-active_model_serializers/error_formatter.rb:8:81: C: Line is too long. [87/80] def call(message, backtrace, options = {}, env = nil, original_exception = nil) ^^^^^^^ 31 files inspected, 2 offenses detected Inspecting 31 files ..CCCCC.C.......C.............. Offenses: lib/grape-active_model_serializers/endpoint_extension.rb:8:3: C: Missing top-level module documentation comment. module EndpointExtension ^^^^^^ lib/grape-active_model_serializers/error_formatter.rb:4:5: C: Missing top-level module documentation comment. module ActiveModelSerializers ^^^^^^ lib/grape-active_model_serializers/error_formatter.rb:8:9: C: Cyclomatic complexity for call is too high. [7/6] def call(message, backtrace, options = {}, env = nil, original_exception = nil) ^^^ lib/grape-active_model_serializers/error_formatter.rb:8:81: C: Line is too long. [87/80] def call(message, backtrace, options = {}, env = nil, original_exception = nil) ^^^^^^^ lib/grape-active_model_serializers/formatter.rb:3:5: C: Missing top-level module documentation comment. module ActiveModelSerializers ^^^^^^ lib/grape-active_model_serializers/options_builder.rb:3:5: C: Missing top-level class documentation comment. class OptionsBuilder ^^^^^ lib/grape-active_model_serializers/serializer_resolver.rb:3:5: C: Missing top-level class documentation comment. class SerializerResolver ^^^^^ lib/grape-active_model_serializers.rb:1:1: C: The name of this source file (grape-active_model_serializers.rb) should use snake_case. require 'active_model_serializers' ^ spec/grape-active_model_serializers_spec.rb:1:1: C: The name of this source file (grape-active_model_serializers_spec.rb) should use snake_case. require 'spec_helper' ^ 31 files inspected, 9 offenses detected Created .rubocop_todo.yml. Run `rubocop --config .rubocop_todo.yml`, or add `inherit_from: .rubocop_todo.yml` in a .rubocop.yml file.```

Yes, and now Rubocop will run clean, commit the changed files.

xn · 2017-10-05T20:20:26Z

kk, constraining to 1.0 works, but breaks the others.

dblock · 2017-10-08T14:27:08Z

Oh I see. Make this conditional, in the case Grape::JSON is defined you can use that, otherwise use MultiJson, which was required in older versions of Grape.

xn · 2017-10-09T15:38:27Z

Finally! I think this is ready to go.

dblock

My only must have is that multi_json dependency, please.

dblock · 2017-10-15T21:52:54Z

grape-active_model_serializers.gemspec

@@ -18,6 +18,7 @@ Gem::Specification.new do |gem|

  gem.add_dependency 'grape', '>= 0.8.0'
  gem.add_dependency 'active_model_serializers', '>= 0.10.0'
+  gem.add_dependency 'multi_json', '~> 1.0'


So, this one should still be removed, it's unnecessary in all Grape versions since it's either coming via Grape or is not used..

dblock · 2017-10-15T21:54:25Z

lib/grape-active_model_serializers/error_formatter.rb

+
+      class << self
+        def call(message, backtrace, options = {}, env = nil, original_exception = nil)
+          result = if respond_to? :present


Nitpick, might be shorter and more concise, but not a must have.

message = present(message) if respond_to?(:present) message = wrap_message(message) message = ...

dblock · 2017-10-15T21:55:08Z

spec/grape-active_model_serializers/error_formatter_spec.rb

+  let(:app) { Class.new(Grape::API) }
+
+  before do
+    ActiveModel::Serializer.config.adapter = :json_api


This pollutes future tests, so should either be set for all tests or unset in after.

dblock · 2017-10-15T21:56:23Z

spec/grape-active_model_serializers/error_formatter_spec.rb

+
+  before do
+    ActiveModel::Serializer.config.adapter = :json_api
+    app.format :json


Move this into the let, so

let(:app) { Class.new(Grape::API) do formatter ... ... end }

dblock · 2017-10-15T21:57:05Z

spec/grape-active_model_serializers/error_formatter_spec.rb

+  describe '#call' do
+    context 'message is an activemodel' do
+      let(:message) {
+        class Foo


This globally defines a class Foo. It should be a Class.new as well.

dblock · 2017-10-15T21:57:47Z

spec/grape-active_model_serializers/error_formatter_spec.rb

+        Foo.new(name: 'bar')
+      }
+      it 'formats the error' do
+        result = subject


This is shared with the example below and should be moved to a let as well.

xn · 2017-10-17T12:21:42Z

This should be it.

dblock · 2017-10-17T14:17:57Z

Code looks great. You still have to add something to README!

xn · 2017-10-17T21:06:16Z

Would this work?

Tell your API to use Grape::Formatter::ActiveModelSerializers

class API < Grape::API
  format :json
  formatter :json, Grape::Formatter::ActiveModelSerializers

  # Serializes errors with ActiveModel::Serializer::ErrorSerializer if an ActiveModel.
  # Serializer conforms to format. ex: json, jsonapi
  error_formatter :json, Grape::Formatter::ActiveModelSerializers
end

dblock · 2017-10-18T13:43:00Z

Something like that, an explain what these output.

xn · 2017-10-18T18:54:41Z

You want me to document the behavior of ActiveModel::Serializer::ErrorSerializer?

dblock · 2017-10-18T23:02:43Z

Yes, something should explain what's happening with this new code that wasn't happening before.

xn · 2017-10-19T13:34:11Z

Does this work?

class API < Grape::API
  format :json
  formatter :json, Grape::Formatter::ActiveModelSerializers

  # Serializes errors with ActiveModel::Serializer::ErrorSerializer if an ActiveModel.
  # Serializer conforms to the adapter, ex: json, jsonapi.
  # So an error formatted with a jsonapi formatter would render as per:
  # http://jsonapi.org/format/#error-objects
  error_formatter :json, Grape::Formatter::ActiveModelSerializers
end

dblock · 2017-10-19T20:28:07Z

Great! As long as there's something in the README.

xn · 2017-10-20T12:17:38Z

That is from the readme.

xn · 2017-10-20T12:18:36Z

Under: Tell your API to use Grape::Formatter::ActiveModelSerializers

dblock · 2017-10-20T14:26:25Z

Oh you're saying we documented it but it was never implemented (sorry, I'm just cheerleading this repo :)).

I'll just merge this, any interest in doing the next release and helping out with this project?

dblock · 2017-10-20T14:26:45Z

And thanks for hanging on with my many nitpicky comments!

xn · 2017-10-22T18:27:06Z

No, that was a miscommunication. Let me do a quick PR for the Docs.

add custom error formatter

60ccc21

xn mentioned this pull request Aug 30, 2017

Custom formatted error messages #77

Closed

Fix rubocop formatting errors

e479e0e

xn added 2 commits September 29, 2017 09:43

update to new method signature

7209dcf

added specs, formatting changes

d90b496

xn added 2 commits October 2, 2017 11:00

have no control over this method signature

ce52109

add multijson

f3aabf1

dblock reviewed Oct 2, 2017

View reviewed changes

CHANGELOG.md Outdated

@@ -2,7 +2,7 @@

### 1.5.2 (Next)

* Your contribution here.

Copy link

Member

dblock Oct 2, 2017

Choose a reason for hiding this comment

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

Put this back.

dblock reviewed Oct 2, 2017

View reviewed changes

xn added 2 commits October 5, 2017 14:47

various fixes

67e22ba

realign merge

dd396ee

xn added 6 commits October 9, 2017 09:05

pivot on Grape::Json

95e1eb1

rubocop fixes

5a22782

get it to work on 0.8

d4b3281

moar rubocop

bc5f61b

please appease the rubocop

1c8eba0

enough already

f2859a9

dblock requested changes Oct 15, 2017

View reviewed changes

style fixes

8a90dae

dblock merged commit b265419 into ruby-grape:master Oct 20, 2017

add custom error formatter #76

add custom error formatter #76

Conversation

xn commented Aug 30, 2017

dblock commented Aug 31, 2017 • edited

xn commented Aug 31, 2017

xn commented Oct 2, 2017

xn commented Oct 2, 2017

dblock commented Oct 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock Oct 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock Oct 3, 2017 • edited

Choose a reason for hiding this comment

xn commented Oct 5, 2017

dblock commented Oct 8, 2017 • edited

xn commented Oct 9, 2017

dblock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xn commented Oct 17, 2017

dblock commented Oct 17, 2017

xn commented Oct 17, 2017

Tell your API to use Grape::Formatter::ActiveModelSerializers

dblock commented Oct 18, 2017

xn commented Oct 18, 2017

dblock commented Oct 18, 2017

xn commented Oct 19, 2017 • edited

dblock commented Oct 19, 2017 • edited

xn commented Oct 20, 2017

xn commented Oct 20, 2017

dblock commented Oct 20, 2017

dblock commented Oct 20, 2017

xn commented Oct 22, 2017

dblock commented Aug 31, 2017 •

edited

dblock Oct 5, 2017 •

edited

dblock Oct 3, 2017 •

edited

dblock commented Oct 8, 2017 •

edited

xn commented Oct 19, 2017 •

edited

dblock commented Oct 19, 2017 •

edited