Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Add support for namespaced input classes #921

Closed
wants to merge 14 commits into from

3 participants

@mikz
Collaborator

Hi there!
First things first - thank you for this awesome project.

I found that using global namespace for custom inputs does not fit our applications, so I made this patch which allows you to namespace custom inputs into custom namespace (form builder class by default).

for example:

class CustomFormBuilder < Formtastic::FormBuilder
  class StringInput < Formtastic::Inputs::StringInput
  end
end

Will use CustomFormBuilder::StringInput instead of formtastic one.

To not break backwards compatibility top level ones are still used first.

Also in the process when writing tests I found out that input helper was not tested for cases when input_classes are on/off so I refactored tests to shared example and let it run for both cases.

Also when doing it I set up some basic mock objects so input helper test can be ran without whole form builder which makes the tests much simpler.

  • namespaced input classes can be in form builder namespace
    • the namespace can be overridden in the form builder
  • upgrade RSpec to get constant stubbing and other goodness
  • refactor part of input helper spec
    • to test behaviour when cache_classes is on/off
    • to not rely on form generation

edit:
I added second commit which allows inheriting input classes from parent form builders.
Unfortunately this feature fails on ruby 1.8. Do you have any ideas how to solve it? Mark it as 1.9 & up only?

edit 2:
I refactored loading code to own class called Formtastic::ClassFinder which encapsulates all the logic shared between ActionHelper and InputHelper. Also moved tests to own spec file.

_edit 3:
Added support for custom actions. See test of 287a0e9. Allows to create own actions like: 'DestroyAction' instead of trying to find class nil.

formtastic.gemspec
@@ -25,7 +25,7 @@ Gem::Specification.new do |s|
s.add_dependency(%q<actionpack>, [">= 3.0"])
- s.add_development_dependency(%q<rspec-rails>, ["~> 2.8.0"])
+ s.add_development_dependency(%q<rspec-rails>, ["~> 2.12.0"])
@justinfrench Owner

Can we leave this dependency alone, tackle in a separate, atomic pull request?

@mikz Collaborator
mikz added a note

done in 18a0682

@justinfrench Owner

… but it's still here :) rebase?

@mikz Collaborator
mikz added a note

right, atomic pull request.. :) issuing one now - #924

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

This is cool, thanks! Why isn't the top-level namespace suitable for your app? I need to build some empathy up for why this is needed in day-to-day applications.

It looks like you're picking the custom namespace to be the name of the configured custom form builder, which I think is a little weird, and likely related to your use case. Normally namespacing is done with a module, not a class.

Perhaps this would be an okay default, but I think introducing a configurable namespace would also be needed to suit a wider audience. Either way, documentation around how this works, where to put your files, how to configure it, etc would be needed before this is accepted, as it's changing/adding to the features of custom inputs and actions.

Finally, ClassFinder feels like a horrible name, although I love that you've split this out. CustomInputOrActionFinder is horrible for other reasons, but at least I get it. Maybe we need two? CustomInputFinder and CustomActionFinder?

Definitely give me some insight into why you need this and keep going ! :)

@mikz
Collaborator

@justinfrench Thanks for checking into this!

We have multiple form builders with some inputs used only there. Some of them are modifying existing inputs, some of them are adding custom ones. We have a lot of forms and changing every form to add some default value or attribute was not really dry, so we went with overriding custom inputs. It worked pretty well in formtastic 1 and we would like to see it in 2.x branch. And the naming - we have form builders in own modules and because these inputs are used only there we want to have them scoped in right class/module/file.

Regarding the form builder class as custom namespace - I wanted some default value to be there and only thing I could think of was the form builder, because everything else is app specific.

Sure, documentation how it works and why to use it and how to do it is required and I'll do it.

I agree that the ClassFinder is terrible name. Creating two classes is fine with me and makes a lot sense, but they need a common base. The implementation of CustomInputFinder and CustomActionFinder will not be long, it should be just ~10 lines of code to do the action/input specific naming. So would you agree on: CustomClassFinder as a base and CustomInputFinder and CustomActionFinder inheriting from it? Or some other name for the base like ClassFinder::Base ?.

@justinfrench
Owner

@mikz sorry for the lag on this. Your proposed CustomInputFinder and CustomActionFinder split sounds great, but I too am struggling to come up with a name for the shared base. Perhaps just rename ClassFinder to CustomInputAndActionFinder — it's also horrible, but more explicit :) Would love to see this merged.

@justinfrench
Owner

@mikz I tried to merge this in manually last night and got pretty stuck. Any chance you could take a look? Other than the name of the ClassFinder class, I'm keen to get this in, but if I haven't heard back soon I'll close it off.

@mikz
Collaborator
@mikz mikz Merge remote-tracking branch 'upstream/master' into namespaced-inputs
Conflicts:
	formtastic.gemspec
	spec/helpers/action_helper_spec.rb
	spec/helpers/input_helper_spec.rb
647cd93
@mikz
Collaborator

@justinfrench In the end I was not satisfied with CustomInputFinder as it does not find just custom inputs, but all of them. So I split it to 3: NamespacedClassFinder, InputClassFinder, ActionClassFinder. I'm quite happy with it. Do you have other suggestions?

Also I had to make tiny changes to make Travis working again.

Otherwise I would say it is done if we agree on the naming.

@justinfrench
Owner

Awesome, will take me a while to review this, but looking good.

@mikz mikz referenced this pull request
Merged

Fix Travis #1035

@mikz
Collaborator

@justinfrench just tell me if you need anything

@vjt vjt commented on the diff
lib/formtastic/namespaced_class_finder.rb
((35 lines not shown))
+ def find_with_const_defined(class_name)
+ @namespaces.find do |namespace|
+ if namespace.const_defined?(class_name)
+ break namespace.const_get(class_name)
+ end
+ end
+ end
+
+ # use auto-loading in development environment
+ def find_by_trying(class_name)
+ @namespaces.find do |namespace|
+ begin
+ break namespace.const_get(class_name)
+ rescue NameError
+ end
+ end
@vjt
vjt added a note

Cool.

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

Closing this in favour of #1043

@mikz mikz deleted the mikz:namespaced-inputs branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 25, 2013
  1. @mikz

    upgrade rspec to 2.12.x

    mikz authored
    * it supports stubbing constants, expects { }, etc.
  2. @mikz
  3. @mikz

    get right input class when inheriting form builders

    mikz authored
    * this unfortunately does not work on Ruby 1.8
      when cache_classes is on
  4. @mikz
  5. @mikz
Commits on Feb 26, 2013
  1. @mikz
Commits on Apr 2, 2014
  1. @mikz

    Merge remote-tracking branch 'upstream/master' into namespaced-inputs

    mikz authored
    Conflicts:
    	formtastic.gemspec
    	spec/helpers/action_helper_spec.rb
    	spec/helpers/input_helper_spec.rb
  2. @mikz
  3. @mikz
Commits on May 5, 2014
  1. @mikz
  2. @mikz

    run travis test with bundler

    mikz authored
  3. @mikz
  4. @mikz
  5. @mikz
This page is out of date. Refresh to see the latest.
View
6 .travis.yml
@@ -5,7 +5,7 @@ before_install:
rvm:
- 1.9.3
- 2.0.0
- - 2.1.0
+ - 2.1.1
gemfile:
- gemfiles/rails_3.2.gemfile
- gemfiles/rails_4.gemfile
@@ -13,7 +13,7 @@ gemfile:
- gemfiles/rails_edge.gemfile
env:
- DEFER_GC=false RAILS_EDGE=true
-script: "rake spec"
+script: "bundle exec rake spec"
matrix:
allow_failures:
- rvm: 1.9.3
@@ -22,6 +22,6 @@ matrix:
- rvm: 2.0.0
gemfile: gemfiles/rails_edge.gemfile
env: DEFER_GC=false RAILS_EDGE=true
- - rvm: 2.1.0
+ - rvm: 2.1.1
gemfile: gemfiles/rails_edge.gemfile
env: DEFER_GC=false RAILS_EDGE=true
View
3  lib/formtastic.rb
@@ -13,6 +13,9 @@ module Formtastic
autoload :LocalizedString
autoload :Localizer
autoload :Util
+ autoload :NamespacedClassFinder
+ autoload :InputClassFinder
+ autoload :ActionClassFinder
if defined?(::Rails) && Util.deprecated_version_of_rails?
::ActiveSupport::Deprecation.warn(
View
12 lib/formtastic/action_class_finder.rb
@@ -0,0 +1,12 @@
+module Formtastic
+ class ActionClassFinder < NamespacedClassFinder
+ def initialize(*)
+ super
+ @namespaces << Formtastic::Actions
+ end
+
+ def class_name(as)
+ "#{super}Action"
+ end
+ end
+end
View
30 lib/formtastic/helpers/action_helper.rb
@@ -90,36 +90,18 @@ def action(method, options = {})
def default_action_type(method, options = {}) #:nodoc:
case method
when :submit then :input
- when :reset then :input
+ when :reset then :input
when :cancel then :link
+ else method
end
end
def action_class(as)
- @input_classes_cache ||= {}
- @input_classes_cache[as] ||= begin
- begin
- begin
- custom_action_class_name(as).constantize
- rescue NameError
- standard_action_class_name(as).constantize
- end
- rescue NameError
- raise Formtastic::UnknownActionError
- end
- end
- end
-
- # :as => :button # => ButtonAction
- def custom_action_class_name(as)
- "#{as.to_s.camelize}Action"
+ @action_class_finder ||= Formtastic::ActionClassFinder.new(self)
+ @action_class_finder[as]
+ rescue Formtastic::ActionClassFinder::NotFoundError
+ raise Formtastic::UnknownActionError, "Unable to find action #{$!.message}"
end
-
- # :as => :button # => Formtastic::Actions::ButtonAction
- def standard_action_class_name(as)
- "Formtastic::Actions::#{as.to_s.camelize}Action"
- end
-
end
end
end
View
43 lib/formtastic/helpers/input_helper.rb
@@ -319,46 +319,11 @@ def column_for(method) #:nodoc:
# input_class(:string) #=> StringInput
# input_class(:awesome) #=> AwesomeInput
def input_class(as)
- @input_classes_cache ||= {}
- @input_classes_cache[as] ||= begin
- Rails.application.config.cache_classes ? input_class_with_const_defined(as) : input_class_by_trying(as)
- end
- end
-
- # prevent exceptions in production environment for better performance
- def input_class_with_const_defined(as)
- input_class_name = custom_input_class_name(as)
-
- if ::Object.const_defined?(input_class_name)
- input_class_name.constantize
- elsif Formtastic::Inputs.const_defined?(input_class_name)
- standard_input_class_name(as).constantize
- else
- raise Formtastic::UnknownInputError, "Unable to find input class #{input_class_name}"
- end
- end
-
- # use auto-loading in development environment
- def input_class_by_trying(as)
- begin
- custom_input_class_name(as).constantize
- rescue NameError
- standard_input_class_name(as).constantize
- end
- rescue NameError
- raise Formtastic::UnknownInputError, "Unable to find input class for #{as}"
- end
-
- # :as => :string # => StringInput
- def custom_input_class_name(as)
- "#{as.to_s.camelize}Input"
+ @input_class_finder ||= Formtastic::InputClassFinder.new(self)
+ @input_class_finder[as]
+ rescue Formtastic::InputClassFinder::NotFoundError
+ raise Formtastic::UnknownInputError, "Unable to find input #{$!.message}"
end
-
- # :as => :string # => Formtastic::Inputs::StringInput
- def standard_input_class_name(as)
- "Formtastic::Inputs::#{as.to_s.camelize}Input"
- end
-
end
end
end
View
12 lib/formtastic/input_class_finder.rb
@@ -0,0 +1,12 @@
+module Formtastic
+ class InputClassFinder < NamespacedClassFinder
+ def initialize(*)
+ super
+ @namespaces << Formtastic::Inputs
+ end
+
+ def class_name(as)
+ "#{super}Input"
+ end
+ end
+end
View
5 lib/formtastic/inputs/base.rb
@@ -23,7 +23,7 @@ def initialize(builder, template, object, object_name, method, options)
# Usefull for deprecating options.
def warn_and_correct_option!(old_option_name, new_option_name)
if options.key?(old_option_name)
- ::ActiveSupport::Deprecation.warn("The :#{old_option_name} option is deprecated in favour of :#{new_option_name} and will be removed from Formtastic in the next version")
+ ::ActiveSupport::Deprecation.warn("The :#{old_option_name} option is deprecated in favour of :#{new_option_name} and will be removed from Formtastic in the next version", caller(6))
options[new_option_name] = options.delete(old_option_name)
end
end
@@ -31,7 +31,7 @@ def warn_and_correct_option!(old_option_name, new_option_name)
# Usefull for deprecating options.
def warn_deprecated_option!(old_option_name, instructions)
if options.key?(old_option_name)
- ::ActiveSupport::Deprecation.warn("The :#{old_option_name} option is deprecated in favour of `#{instructions}` and will be removed in the next version")
+ ::ActiveSupport::Deprecation.warn("The :#{old_option_name} option is deprecated in favour of `#{instructions}` and will be removed in the next version", caller(6))
end
end
@@ -77,4 +77,3 @@ def removed_option!(old_option_name)
end
end
end
-
View
53 lib/formtastic/namespaced_class_finder.rb
@@ -0,0 +1,53 @@
+module Formtastic
+ class NamespacedClassFinder
+ DEFAULT_NAMESPACE = ::Object
+
+ attr_reader :namespaces
+
+ # @private
+ class NotFoundError < NameError
+ end
+
+ def initialize(builder)
+ @namespaces = [::Object, builder.class]
+ @cache = {}
+ end
+
+ def [](as)
+ @cache[as] ||= find(as)
+ end
+
+ def finder_method
+ ::Rails.application.config.cache_classes ? :find_with_const_defined : :find_by_trying
+ end
+
+ def find(as, method = finder_method)
+ class_name = class_name(as)
+
+ __send__(method, class_name) or raise NamespacedClassFinder::NotFoundError, "class #{class_name}"
+ end
+
+ def class_name(as)
+ "#{as.to_s.camelize}"
+ end
+
+ # prevent exceptions in production environment for better performance
+ def find_with_const_defined(class_name)
+ @namespaces.find do |namespace|
+ if namespace.const_defined?(class_name)
+ break namespace.const_get(class_name)
+ end
+ end
+ end
+
+ # use auto-loading in development environment
+ def find_by_trying(class_name)
+ @namespaces.find do |namespace|
+ begin
+ break namespace.const_get(class_name)
+ rescue NameError
+ end
+ end
@vjt
vjt added a note

Cool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ end
+ end
+end
View
14 spec/action_class_finder_spec.rb
@@ -0,0 +1,14 @@
+# encoding: utf-8
+require 'spec_helper'
+require 'formtastic/action_class_finder'
+
+describe Formtastic::ActionClassFinder do
+ include FormtasticSpecHelper
+
+ let(:builder) { Formtastic::FormBuilder.allocate }
+ subject(:finder) { Formtastic::ActionClassFinder.new(builder) }
+
+ it 'has correct namespaces' do
+ expect(finder.namespaces).to eq([Object,Formtastic::FormBuilder, Formtastic::Actions])
+ end
+end
View
31 spec/helpers/action_helper_spec.rb
@@ -252,7 +252,13 @@
end
describe 'instantiating an action class' do
-
+ it "should delegate to ClassFinder" do
+ concat(semantic_form_for(@new_post) do |builder|
+ Formtastic::ActionClassFinder.any_instance.should_receive(:[]).with(:button).and_call_original
+ builder.action(:submit, :as => :button)
+ end)
+ end
+
context 'when a class does not exist' do
it "should raise an error" do
lambda {
@@ -262,7 +268,17 @@
}.should raise_error(Formtastic::UnknownActionError)
end
end
-
+
+ context 'of unknown action' do
+ it "should try to load class named as the action" do
+ expect {
+ semantic_form_for(@new_post) do |builder|
+ builder.action(:destroy)
+ end
+ }.to raise_error(Formtastic::UnknownActionError, 'Unable to find action class DestroyAction')
+ end
+ end
+
context 'when a customized top-level class does not exist' do
it 'should instantiate the Formtastic action' do
@@ -289,20 +305,17 @@ class ::ButtonAction < Formtastic::Actions::ButtonAction
end)
end
end
-
+
describe 'when instantiated multiple times with the same action type' do
-
- it "should be cached (not calling the internal methods)" do
- # TODO this is really tied to the underlying implementation
+ it "should be cached" do
concat(semantic_form_for(@new_post) do |builder|
- builder.should_receive(:custom_action_class_name).with(:button).once.and_return(::Formtastic::Actions::ButtonAction)
+ Formtastic::ActionClassFinder.should_receive(:new).once.and_call_original
builder.action(:submit, :as => :button)
builder.action(:submit, :as => :button)
end)
end
-
end
-
+
describe 'support for :as on each action' do
it "should raise an error when the action does not support the :as" do
View
13 spec/helpers/input_helper_spec.rb
@@ -878,7 +878,6 @@ def length_should_be_required(options)
end
describe 'instantiating an input class' do
-
context 'when a class does not exist' do
it "should raise an error" do
lambda {
@@ -889,6 +888,14 @@ def length_should_be_required(options)
end
end
+ it "should delegate to ClassFinder" do
+ concat(semantic_form_for(@new_post) do |builder|
+ Formtastic::InputClassFinder.any_instance.should_receive(:[]).
+ with(:string).and_call_original
+ builder.input(:title, :as => :string)
+ end)
+ end
+
context 'when a customized top-level class does not exist' do
it 'should instantiate the Formtastic input' do
@@ -921,7 +928,7 @@ class ::StringInput < Formtastic::Inputs::StringInput
it "should be cached (not calling the internal methods)" do
# TODO this is really tied to the underlying implementation
concat(semantic_form_for(@new_post) do |builder|
- builder.should_receive(:custom_input_class_name).with(:string).once.and_return(::Formtastic::Inputs::StringInput)
+ Formtastic::InputClassFinder.should_receive(:new).once.and_call_original
builder.input(:title, :as => :string)
builder.input(:title, :as => :string)
end)
@@ -930,6 +937,4 @@ class ::StringInput < Formtastic::Inputs::StringInput
end
end
-
end
-
View
14 spec/input_class_finder_spec.rb
@@ -0,0 +1,14 @@
+# encoding: utf-8
+require 'spec_helper'
+require 'formtastic/input_class_finder'
+
+describe Formtastic::InputClassFinder do
+ include FormtasticSpecHelper
+
+ let(:builder) { Formtastic::FormBuilder.allocate }
+ subject(:finder) { Formtastic::InputClassFinder.new(builder) }
+
+ it 'has correct namespaces' do
+ expect(finder.namespaces).to eq([Object,Formtastic::FormBuilder, Formtastic::Inputs])
+ end
+end
View
78 spec/namespaced_class_finder_spec.rb
@@ -0,0 +1,78 @@
+# encoding: utf-8
+require 'spec_helper'
+require 'formtastic/namespaced_class_finder'
+
+describe Formtastic::NamespacedClassFinder do
+ include FormtasticSpecHelper
+
+ let(:builder) { Formtastic::FormBuilder.allocate }
+ subject(:finder) { Formtastic::NamespacedClassFinder.new(builder) }
+
+
+ shared_examples 'Namespaced Class Finder' do
+ let(:as) { :custom_class }
+ let(:class_name ) { 'CustomClass'}
+ let(:fake_class) { double('FakeClass') }
+
+
+ subject(:found_class) { finder.find(as) }
+
+ let(:namespaces) { [ Object, ] }
+
+ context 'when first namespace is defined' do
+ before do
+ stub_const(class_name, fake_class)
+ end
+
+ it do
+ expect(found_class).to be(fake_class)
+ end
+ end
+
+ context 'when second namespace is defined' do
+ before do
+ stub_const('Formtastic::FormBuilder::' + class_name, fake_class)
+ end
+
+ it do
+ expect(found_class).to be(fake_class)
+ end
+ end
+ end
+
+
+ context '#finder_method' do
+ subject { finder.finder_method }
+
+ before do
+ Rails.application.config.stub(:cache_classes).and_return(cache_classes)
+ end
+
+ context 'when cache_classes is on' do
+ let(:cache_classes) { true }
+ it_behaves_like 'Namespaced Class Finder'
+
+ it { should eq(:find_with_const_defined) }
+ end
+
+ context 'when cache_classes is off' do
+ let(:cache_classes) { false }
+ it_behaves_like 'Namespaced Class Finder'
+
+ it { should eq(:find_by_trying) }
+ end
+ end
+
+ context '#[]' do
+ it 'caches calls' do
+ expect(subject).to receive(:find).once.and_call_original
+ subject[:object]
+ subject[:object]
+ end
+ end
+
+
+ context '#find_with_const_defined' do
+ subject(:finder) { Formtastic::NamespacedClassFinder.new([])}
+ end
+end
Something went wrong with that request. Please try again.