Add library configuration object #17

ezekg · 2016-12-19T20:23:06Z

Implemented along with this is a configuration option to define a custom parameter parser for the :jsonapi mime-type, something which I think is a needed addition.

houndci-bot · 2016-12-19T20:23:21Z

spec/config_spec.rb

+  end
+
+  context 'when a custom jsonapi parameter parser is used' do
+    let(:parser) { lambda {} }


Use the -> { ... } lambda literal syntax for single line lambdas.

houndci-bot · 2016-12-19T20:23:22Z

spec/config_spec.rb

@@ -0,0 +1,23 @@
+require 'rails_helper'


Missing frozen string literal comment.

houndci-bot · 2016-12-19T20:23:22Z

lib/jsonapi/rails/railtie.rb

          else
-            ::ActionDispatch::ParamsParser::DEFAULT_PARSERS[Mime[:jsonapi]] = PARSER
+            ::ActionDispatch::ParamsParser::DEFAULT_PARSERS[Mime[:jsonapi]] = JSONAPI::Rails.config.parameter_parser


Line is too long. [116/80]

houndci-bot · 2016-12-19T20:23:22Z

lib/jsonapi/rails/railtie.rb

@@ -23,9 +22,9 @@ class Railtie < ::Rails::Railtie
          Mime::Type.register MEDIA_TYPE, :jsonapi

          if ::Rails::VERSION::MAJOR >= 5
-            ::ActionDispatch::Request.parameter_parsers[:jsonapi] = PARSER
+            ::ActionDispatch::Request.parameter_parsers[:jsonapi] = JSONAPI::Rails.config.parameter_parser


Line is too long. [106/80]

houndci-bot · 2016-12-19T20:23:23Z

lib/jsonapi/rails/configuration.rb

+      }
+    }.freeze
+
+    def self.configure(&block)


Unused method argument - block. If it's necessary, use _ or _block as an argument name to indicate that it won't be used. You can also write as configure(*) if you want the method to accept any arguments but don't care about them.

houndci-bot · 2016-12-19T20:23:23Z

lib/jsonapi/rails/configuration.rb

+    class Configuration < ActiveSupport::InheritableOptions; end
+
+    DEFAULT_CONFIG = {
+      parameter_parser: lambda { |body|


Avoid using {...} for multi-line blocks.

houndci-bot · 2016-12-19T20:23:23Z

lib/jsonapi/rails/configuration.rb

@@ -0,0 +1,21 @@
+module JSONAPI
+  module Rails


Missing top-level module documentation comment.

houndci-bot · 2016-12-19T20:23:23Z

lib/jsonapi/rails/configuration.rb

@@ -0,0 +1,21 @@
+module JSONAPI


Missing frozen string literal comment.

beauby · 2016-12-20T03:01:26Z

Thanks for the PR @ezekg! How do you feel about a simple boolean option to prevent registering the params parser? (e.g. JSONAPI::Rails.configure { |config| config.register_params_parser = false })

ezekg · 2016-12-20T04:25:24Z

I actually experimented with that first, along with adding an option for toggling the registration of the jsonapi mime type in case it's being registered another way. But the library depends on both of those being defined a certain way (jsonapi mime type being under :jsonapi, instead of say an alias for the :json mime type). So in the end, both of those can't really be 'disabled' (instead, they're just defined elsewhere), so I figured exposing the definition itself was the best option.

Would you rather implement toggles instead of exposing the params parser?

beauby · 2016-12-21T12:57:16Z

@ezekg Right, so rails expects a params parser for the application/vnd.api+json mime type once it is registered (which jsonapi-rails does). The reason why I favor a boolean config to avoid declaring a params parser is that an existing app with a custom params parser for the jsonapi mime type wouldn't have to move that parser definition.

Ideally, jsonapi-rails should allow a more fine-grained configuration of what it declares in the initializers (registering params parser, registering renderer), and ensure it does not redefine the :jsonapi mime type if already defined.

ezekg · 2016-12-21T14:18:55Z

Okay yeah, that sounds good to me. I'll modify the PR to implement it that way.

Implements a global configuration object so that we can conditionally register things such as the renderer, parameter parser and mime type.

houndci-bot · 2016-12-22T14:59:58Z

lib/jsonapi/rails/railtie.rb

+            if ::Rails::VERSION::MAJOR >= 5
+              ::ActionDispatch::Request.parameter_parsers[:jsonapi] = PARSER
+            else
+              ::ActionDispatch::ParamsParser::DEFAULT_PARSERS[Mime[:jsonapi]] = PARSER


Line is too long. [86/80]

houndci-bot · 2016-12-22T14:59:58Z

lib/jsonapi/rails/railtie.rb

+            if ::Rails::VERSION::MAJOR >= 5
+              ::ActionDispatch::Request.parameter_parsers[:jsonapi] = PARSER
+            else
+              ::ActionDispatch::ParamsParser::DEFAULT_PARSERS[Mime[:jsonapi]] = PARSER


Line is too long. [86/80]

houndci-bot · 2016-12-22T14:59:59Z

lib/jsonapi/rails/configuration.rb

+      register_renderers: true
+    }.freeze
+
+    def self.configure(&block)


Unused method argument - block. If it's necessary, use _ or _block as an argument name to indicate that it won't be used. You can also write as configure(*) if you want the method to accept any arguments but don't care about them.

houndci-bot · 2016-12-22T14:59:59Z

lib/jsonapi/rails/configuration.rb

+      register_renderers: true
+    }.freeze
+
+    def self.configure(&block)


Unused method argument - block. If it's necessary, use _ or _block as an argument name to indicate that it won't be used. You can also write as configure(*) if you want the method to accept any arguments but don't care about them.

ezekg · 2016-12-22T15:15:16Z

First pass on the changes we discussed. Do we want to implement any validation to ensure that the mime-type/parser is never redefined? (Or error handling in the cases where they aren't defined?) I'm now able to implement like so, which is great:

# config/initializers/jsonapi.rb

# Register JSON API mime-type
Mime::Type.register "application/vnd.api+json", :jsonapi

# Register parameter parser for JSON API mime-type to transform param keys from
# camel case to snake case
ActionDispatch::Request.parameter_parsers[:jsonapi] = -> (raw_post) {
  data = ActiveSupport::JSON.decode raw_post
  data = { _json: data } unless data.is_a? Hash
  data.deep_transform_keys! &:underscore
  data.with_indifferent_access
}

# Make sure jsonapi/rails doesn't overwrite our registrations
JSONAPI::Rails.configure do |config|
  config.register_parameter_parser = false
  config.register_mime_type = false
end

beauby · 2016-12-22T17:08:52Z

Safeguards for not overriding already registered mime type/params parser seems like a good idea (we just have to make sure that jsonapi-rb's code is ran after the user-supplied rails initializer) .
Regarding warnings for when they're not, I think it is not necessary as long as the default behavior is to register them.

ezekg · 2016-12-22T17:53:29Z

Yeah, so right now it looks like we're hooking into the controller setup, which I believe is after user-supplied initializers are run. Is that correct? If so, we should be safe to do a little bit of validation.

beauby · 2017-03-16T12:34:02Z

hooking into the controller setup, which I believe is after user-supplied initializers are run. Is that correct?

I don't know, but I'll look into it. Other than that, this PR looks really good, apologies for taking literally months to review it 🙄

ezekg · 2017-03-16T14:28:44Z

Haha It's cool. I've been using my fork in prod for a few months and things are good. 👍

ezekg · 2017-04-13T04:05:19Z

Are you still wanting to move forward on this, @beauby?

beauby · 2017-04-13T14:48:00Z

@ezekg Yes, definitely. I just want to take care of ActiveModel::Errors serialization first, but this is on my radar!

houndci-bot · 2017-07-25T08:19:37Z

spec/config_spec.rb

@@ -0,0 +1,39 @@
+require 'rails_helper'
+
+RSpec.describe JSONAPI::Rails.config do


Block has too many lines. [29/25]

houndci-bot · 2017-07-25T08:19:37Z

spec/config_spec.rb

@@ -0,0 +1,39 @@
+require 'rails_helper'


Missing magic comment # frozen_string_literal: true.

houndci-bot · 2017-07-25T08:19:37Z

lib/jsonapi/rails/configuration.rb

@@ -0,0 +1,19 @@
+module JSONAPI


Missing magic comment # frozen_string_literal: true.

beauby · 2017-07-25T08:24:10Z

The time has finally come 🙏
Thanks again for this @ezekg!

* upstream/master: Conditionally include controller mixin (jsonapi-rb#40) Add initializer generator. (jsonapi-rb#39) Add ActiveSupport::Notifications instrumentation. (jsonapi-rb#41) Add library configuration object (jsonapi-rb#17) Rename JSONAPI::Rails::ActionController to JSONAPI::Rails::Controller. (jsonapi-rb#38) Add hook for default jsonapi object. (jsonapi-rb#37) Add hook for default exposures. (jsonapi-rb#36) Add hook for automatic pagination links. (jsonapi-rb#35) Upgrade to new jsonapi-deserializable version. (jsonapi-rb#34) Prepare for v0.2.1 release

houndci-bot reviewed Dec 19, 2016

View reviewed changes

spec/config_spec.rb

@@ -0,0 +1,23 @@

require 'rails_helper'

Copy link

houndci-bot Dec 19, 2016

Choose a reason for hiding this comment

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

Missing frozen string literal comment.

houndci-bot reviewed Dec 19, 2016

View reviewed changes

lib/jsonapi/rails/configuration.rb

@@ -0,0 +1,21 @@

module JSONAPI

module Rails

Copy link

houndci-bot Dec 19, 2016

Choose a reason for hiding this comment

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

Missing top-level module documentation comment.

houndci-bot reviewed Dec 19, 2016

View reviewed changes

lib/jsonapi/rails/configuration.rb

@@ -0,0 +1,21 @@

module JSONAPI

Copy link

houndci-bot Dec 19, 2016

Choose a reason for hiding this comment

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

Missing frozen string literal comment.

Add library configuration object

d9ef54a

Implements a global configuration object so that we can conditionally register things such as the renderer, parameter parser and mime type.

ezekg force-pushed the master branch from 3ff3eb5 to d9ef54a Compare December 22, 2016 14:59

houndci-bot reviewed Dec 22, 2016

View reviewed changes

Merge branch 'master' into master

dcd4259

houndci-bot reviewed Jul 25, 2017

View reviewed changes

Update configuration.rb

7ff5275

Update config_spec.rb

4572df4

beauby merged commit 0d513ca into jsonapi-rb:master Jul 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add library configuration object #17

Add library configuration object #17

ezekg commented Dec 19, 2016

houndci-bot Dec 19, 2016

houndci-bot Dec 19, 2016

houndci-bot Dec 19, 2016

houndci-bot Dec 19, 2016

houndci-bot Dec 19, 2016

houndci-bot Dec 19, 2016

houndci-bot Dec 19, 2016

houndci-bot Dec 19, 2016

beauby commented Dec 20, 2016

ezekg commented Dec 20, 2016

beauby commented Dec 21, 2016

ezekg commented Dec 21, 2016

houndci-bot Dec 22, 2016

houndci-bot Dec 22, 2016

houndci-bot Dec 22, 2016

houndci-bot Dec 22, 2016

ezekg commented Dec 22, 2016 •

edited

Loading

beauby commented Dec 22, 2016

ezekg commented Dec 22, 2016

beauby commented Mar 16, 2017

ezekg commented Mar 16, 2017

ezekg commented Apr 13, 2017

beauby commented Apr 13, 2017

houndci-bot Jul 25, 2017

houndci-bot Jul 25, 2017

houndci-bot Jul 25, 2017

beauby commented Jul 25, 2017

		@@ -0,0 +1,39 @@
		require 'rails_helper'

		RSpec.describe JSONAPI::Rails.config do

Add library configuration object #17

Add library configuration object #17

Conversation

ezekg commented Dec 19, 2016

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

Choose a reason for hiding this comment

beauby commented Dec 20, 2016

ezekg commented Dec 20, 2016

beauby commented Dec 21, 2016

ezekg commented Dec 21, 2016

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

ezekg commented Dec 22, 2016 • edited Loading

beauby commented Dec 22, 2016

ezekg commented Dec 22, 2016

beauby commented Mar 16, 2017

ezekg commented Mar 16, 2017

ezekg commented Apr 13, 2017

beauby commented Apr 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beauby commented Jul 25, 2017

ezekg commented Dec 22, 2016 •

edited

Loading