Data attributes, RSpec profiling, Guard and more #673

Closed
wants to merge 88 commits into
from

Projects

None yet
@jcf
jcf commented Sep 1, 2011

This is a reasonably large pull request that adds the ability to use…

= f.input :name, data: {foo: 'bar'}

…rather than…

= f.input :name input_html: {data: {foo: 'bar'}}

(Currently with the exception of with checkboxes, more on that below)

The reason it is large is because in the process of making this change I've also done the following.

  • Added RSpec profiling to make it easier to spot slow specs
  • Added Guard RSpec to support TDD
  • Moved Formtastic specs in to spec/formtastic to follow RSpec convention
  • Fixed bundle console by adding require 'active_support' to lib/formtastic.rb
  • Changed an I18n spec that I believe was testing something different to what was described
  • Removed some trailing whitespace in lib/formtastic/inputs/base.rb

All the changes are as atomic as I could make them. Naturally the small change I made to pass the :data straight through to Rails is somewhat dependent on the new location of the spec files.

There is one failing spec because of checkboxes and the non-standard approach to generating input_html_options. I plan to fix this but wanted to discuss the approach before proceeding as from the TODO I'm guessing you might already have thoughts on how you want to do this.

I noticed the issue you created regarding speeding up tests and decided to not use mock_everything in the data_attributes_spec. The reason being mock_everything does a lot of work that isn't required in the new specs and creating less mocks and stubs should help speed things up.

Let me know if you want this pull request broken up. I'm happy to rebase away commits and push new branches.

Regards,

James

@jcf jcf commented on an outdated diff Sep 1, 2011
spec/formtastic/inputs/data_attributes_spec.rb
@@ -0,0 +1,90 @@
+require 'spec_helper'
+
+describe 'Data Attributes' do
+ include FormtasticSpecHelper
+
+
+ def pend_unless_country_select_available(builder, method_name)
@jcf
jcf Sep 1, 2011

This is a bit smelly. I thought about not testing country_select but if it's installed there's no harm in running the tests. Perhaps a next would be preferred to use of pending though.

@jcf jcf commented on an outdated diff Sep 1, 2011
spec/formtastic/inputs/data_attributes_spec.rb
+ end
+ end
+
+ before do
+ @output_buffer = ''
+ end
+
+ formtastic_inputs.each do |klass, method_name|
+
+ describe klass.to_s do
+
+ context 'when data attributes are present' do
+
+ let(:field_with_data) do
+ lambda { |builder|
+ pend_unless_country_select_available(builder, method_name)
@jcf
jcf Sep 1, 2011

The more I look at this the more I want to refactor it. It's pretty horrible. :)

@justinfrench
Owner

Hi James, looks good, although rather epic :)

I'll be cutting a 2-0-stable branch as soon as I ship 2.0, after which we can look at merging this into master, assuming you're happy to keep polishing and rebasing here for a bit.

The only commit I think should be kept very separate or perhaps reverted entirely is the shifting of all spec paths — it's the sort of patch that needs everyone on the same page and all major branches merged in order to ease the pain. It already doesn't cleanly apply :) Is there a reason other than convention to do it?

In regards to the stuff in checkboxes, not sure exactly which TODO you mean... Which file/line?

I might have to give you commit rights after this monster :D

@jcf
jcf commented Sep 2, 2011

Hi Justin,

I'm happy to rebase this every so often to keep it up-to-date. Not sure why it doesn't apply cleanly at the moment though. Perhaps there was a commit made after I forked from master. I'm sure it won't be a problem to resolve though.

The failing test is due to the inconsistent behaviour used when generating input_html_options for a hidden input. Sorry about that, it's nothing to do with checkboxes. It should be easy enough to fix but it was midnight when I came across it and had to call it a night.

Plugins and libraries like Guard::Rspec and Rake.vim expect spec files to mirror lib files in gems, which is why I moved the specs.

A small change to the Guardfile would fix this but something like Rake.vim can not, as far as I'm aware, look anywhere but in a mirrored location. I'm not sure how other editors that have a jump to spec feature (TextMate, Sublime Text etc.) behave. Perhaps they have a more forgiving lookup path and instead of moving the specs I can ask @tpope to change Rake.vim to look in more than one place.

I can remove the spec move commit if you'd prefer. It will cause issues for anyone with a long running feature branch when they come to rebase or merge.

@tpope
tpope commented Sep 2, 2011

@jcf it already looks 4 places: test/, spec/, test/unit/, and spec/unit/. I looked at a couple of dozen gems gems when I first wrote rake.vim and those were the recurring structures. The rest were all over the map. Point me at 3 unrelated gems that use the same pattern and I'll add support for it.

And I agree with @justinfrench that a massive rearrange doesn't belong in the middle of an unrelated pull request. :)

@tpope
tpope commented Sep 2, 2011

But I would add that a rename on master shouldn't be the end of the world. Git supports merge through rename.

@jcf
jcf commented Sep 3, 2011

I've removed the spec move from this pull request for now.

@tpope Thanks for responding. I think there are two common patterns used in Ruby gems with RSpec. For a Ruby file in lib/a/b/implementation.rb there are two typical locations of a spec.

  1. spec/a/b/implementation_spec.rb
  2. spec/b/implementation_spec.rb

The latter is used in WebMock, Formtastic, Sunspot, and I'm sure a fair few other gems. It's not the convention but it is out there. I don't think I've ever seen `spec/unit used.

@justinfrench
Owner

I'm keen to pull most of this in, but it needs a rebase, and I'd really prefer some smaller pull requests or commits. At the very least, can I convince you to rebase? :)

@jcf
jcf commented Dec 18, 2011

I've rebased and fixed a couple of failing tests.

If you want me to break this pull request up I'm happy to do so. Maybe you could list the commits you want and the ones you don't and I'll get rid of the ones you don't.

  • I've fixed the FormtasticInputs helper I added. Essentially I've made it stricter in order to prevent testing objects that aren't inputs.
  • I've removed a couple of autoloads in Formtastic::Inputs as it appears the constants have gone. They were Basic and NumericInput. If this is wrong please let me know and I'll rip the commit out of this pull request.

Regards.

@justinfrench
Owner

@jcf Thanks!

I've made a comment on 9ad8699, so there's that. There's a few comments of your own that you might want to review before this stuff is in master (?).

Ultimately, all this stuff is probably a separate request, but perhaps the simplest thing is to just make sure you're happy with your work, and then pull it all in.

mattvague and others added some commits Sep 1, 2011
@mattvague @jcf mattvague Closes #667 Made it easier to override commit button wrapper class by…
… splitting the classes into a different method, added missing wrapper class to spec
3aa5bf7
@mattvague @jcf mattvague Refactored commit_button method. Split it into a couple of different …
…methods to make it easier to read and make it more customizable
98227f8
@eneagoe @jcf eneagoe collection_from_association now works with Proc conditions for the as…
…sociations
46b3e0b
@ovelar @jcf ovelar Added form template for Slim engine 159afe5
@cthiel @jcf cthiel Closes #674 don't check all check_boxes if Array is passed to :collec…
…tion
c9142a8
@cthiel @jcf cthiel Revert "Closes #674 don't check all check_boxes if Array is passed to…
… :collection"

This reverts commit bd218ed.
81bc96b
@cthiel @jcf cthiel add failing spec for issue #674 5c80339
@cthiel @jcf cthiel Closes #674 don't check all check_boxes if Array is passed to :collec…
…tion
570a84c
@taavo @jcf taavo add failing spec for 677 select generates incorrect name when passed …
…nil (only fails prior to 3.0.10)
54233d8
@hypomodern @jcf hypomodern MongoMapper does not use `associations(method)`. `.associations` is a…
…n accessor.
a271f55
@hypomodern @jcf hypomodern this should fix the remaining problems. ffa79ba
@crystalin @jcf crystalin fix nested object i18n scopes order
add failing spec for issue #613
5348bac
@chadoh @jcf chadoh very minor grammar fix e50797c
@justinfrench @jcf a few README clean-ups for 2.0 21d645f
@justinfrench @jcf version bump 7b877a3
@alsemyonov @jcf alsemyonov Depend on Railties instead of Rails
Fix automatic dependency on ActiveRecord whether application uses it or another ORM/ODM.
bc91e1b
@justinfrench @jcf 2.0.0 Changelog 2549b62
@justinfrench @jcf more changelog'n f1eb575
@justinfrench @jcf fixed #690 :input_html => { :multiple => true} had no affect 95f6012
@alsemyonov @jcf alsemyonov Depend in runtime only on ActionPack 8c45133
@justinfrench @jcf restore support for :multiple option, didn't have to change specs, wh…
…ich is a concern, ref #690
721391e
@sobrinho @jcf sobrinho Remove active resource require because we are not using it 2112154
@sobrinho @jcf sobrinho Use `require 'bundler/setup'` instead of calling Bundler.setup manually ea07565
@sobrinho @jcf sobrinho Add tzinfo as development dependency
I guess this dependency is defined on wrong place on rails
c7ae0e6
@sobrinho @jcf sobrinho Do not need to require active_record in this spec b88cd88
@sobrinho @jcf sobrinho Do not verify coverage by default
Seems like rcov needs active record but do not declare it as dependency
on gem spec.

I will check this later.
ceb92b0
@michaelkoper @jcf michaelkoper Fixed placeholder tests for all input types (not just string). Added …
…placeholders for textareas.
79c5153
@michaelkoper @jcf michaelkoper deleted before block in splaceholder spec. 38f5b83
@jcf Nathan Long Improve grammar, capitalization, formatting 1fc73bc
@justinfrench @jcf spec to expose that :multiple => true doesn't work when there's no as…
…sociation ref #690
ba70370
@justinfrench @jcf fixes #690 and failing test in 4f6e4b9 12b4636
@jcf Nathan Long Better documentation for custom inputs 1a954a3
@justinfrench @jcf always use inputs() block in README examples, less chance of WTF moments 180bbf1
@justinfrench @jcf use |f| consistently in README for form builder variable name 6800712
@jcf Nathan Long Extract method field_set_legend.
Extracted from field_set_and_list_wrapping.
This lets us override how the legend is constructed
without affecting the rest of the fieldset. Also
makes field_set_and_list_wrapping a little more concise.
9a57737
@alexrothenberg @jcf alexrothenberg added generator specs using ammeter gem 06d456e
@twalpole @jcf twalpole Fix issue with the extraction of field_set_legend 1931ed0
@justinfrench @jcf CHANGELOG b77dd0b
@justinfrench @jcf verion bump 2.0.1 8c4caf4
@dim @jcf dim Fixed prompts on select inputs 474e834
@sobrinho @jcf sobrinho Revert "Fixed prompts on select inputs"
This reverts commit 5cec1a6.
99006d1
@dim @jcf dim Improved :prompt patch for select inputs b5c2717
@jcf Nathan Long Ability to customize localization
- `localized_string` uses a configurable class
to perform lookups
- FormBuilder.i18n_localizer is Formtastic::Localizer
by default; this maintains backwards compatability
- Updated documentation and generator accordingly
309b6a4
@jcorcuera @jcf jcorcuera FormGenerator is Back!
It is based on the old FormGenerator and keeps the same configuration options.
c82e160
@jcorcuera @jcf jcorcuera Fix generator templates: To generate the correct indentation for Haml…
… and Slim form partials.
27da070
@jnimety @jcf jnimety memoize the selected values array
Signed-off-by: Joel Nimety <jnimety@continuity.net>
156d8de
@jnimety @jcf jnimety better code organization, thanks sobrinho
Signed-off-by: Joel Nimety <jnimety@continuity.net>
d757675
@haines @jcf haines Fixed invalid html output for nested inputs with multiple siblings 3bec1f0
@haines @jcf haines 1.8 hash syntax d821d0f
@justinfrench @jcf thin out README documentation from pull #716 a57e205
@justinfrench @jcf tweak localizer documentation and shift up to class level for Yard (r…
…ef pull #716)
a76dd86
@justinfrench @jcf thin out config template documentation for localizer from pull #716 3187a02
@jcf Tom Miller Update commit_button documentation c28c0df
@justinfrench @jcf remove deprecated SemanticFormBuilder 7ccfc6f
@justinfrench @jcf remove deprecated SemanticFormHelper eb97771
@justinfrench @jcf remove deprecated inline_errors_for and related methods 3abb578
@justinfrench @jcf remove deprecated :as => :numeric (use number instead) a3f3897
@justinfrench @jcf Removed deprecated :label_method, :value_method & :group_label_method…
… options.
869d488
@justinfrench @jcf whitespace 55d2531
@justinfrench @jcf fix boolean input to respect :index from fields_for in input[name] at…
…tribute, ref #711
6ef89b4
@justinfrench @jcf fix select input to respect :index from fields_for in select[name] at…
…tribute, ref #711
c835c9d
@justinfrench @jcf fix checkboxes input to respect :index from fields_for in input[name]…
… and [id] attributes, ref #711
ce22ec5
@justinfrench @jcf fix date, datetime and time inputs to respect :index from fields_for,…
… ref #711
444c04f
@jslag @jcf jslag address syntax errors with 1.9.2-p290
The syntax introduced in 253dc6d, eg.

  concat(builder.fields_for :author, :index => 3 do |author|

was producing these errors:

  syntax error, unexpected keyword_do_block, expecting ')' (SyntaxError)
fc26aa0
@jslag @jcf jslag Support empty Time inputs
Before this change, it wasn't possible to create an input of type 'time'
that didn't default to the current date. This made it impossible to
differentiate between a form submission wherein the user hadn't picked a
date, and one in which they'd picked midnight UTC.
babe110
@justinfrench @jcf coverage to assert that fields for with :index works on various input…
…s, ref #711
4d343a0
@jnimety @jcf jnimety calculate length of integer columns as bytes
Signed-off-by: Joel Nimety <jnimety@continuity.net>
f6e8fd8
@justinfrench @jcf passing spec related to #714 43b8cbc
@jcf jcf Add profiling to RSpec c150982
@jcf jcf Add Guard RSpec to ease TDD 3b87966
@jcf jcf Require ActiveSupport in formtastic.rb
Without the require `bundle console` does not work. Adding the require
fixes this.
f8ebff7
@jcf jcf Test using a default when a translation is missing
Reword the I18n default spec to be a bit more descriptive and test that
the default option is returned when asking I18n for a translation that
does not exist.
06e02b4
@jcf jcf Strip trailing whitespace from Inputs::Base fb1291b
@jcf jcf Make accessing Rails data attributes sugar easier 327a064
@kazjote @jcf kazjote Documentation update: :include_blank option should be passed outside …
…of :input_html option in order to take effect while using f.input :as => :select.
85160e5
@bradleypriest @jcf bradleypriest Fixed up the documentation for text_input 8880c90
@spraints @jcf spraints Demonstrate a problem when using with_options.
A failing test.
7bc105b
@spraints @jcf spraints Clone wrapper_html options before modifying it.
A passing test.
a650a00
@rgarner @jcf rgarner Fix for #740 - Mongoid belongs_to select raises NoMethodError and alw…
…ays returns empty results
8a68cc2
@justinfrench @jcf fix documentation example on PhoneInput 20d622a
@justinfrench @jcf cols => size in README, fixes #753 8ff83c5
@brentvatne @jcf brentvatne Fix README to reference the correct filepath (config/initializers) fo…
…r the formtastic.rb config file.
2378bcd
@nashby @jcf nashby use id for commit_button like rails2 did it, closes #686 fb6b8bf
@nashby @jcf nashby allow to use numbers in collection for radio and check_boxes, closes #… a8f4f8c
@jcf jcf Make FormtasticInputs used in specs more strict
Previously constants within `Formtastic::Inputs` were used to determine
what to test. There are some constants within `Inputs` that should be
ignored.

The approach now is to glob files in `lib/formtastic/inputs` and pull
out any that end in `_input.rb`.
1a96409
@jcf jcf Remove unused autoloads in Formtastic::Inputs 8ca4b21
@jcf jcf Bring back Formatastic::Inputs::Base#warn_and_correct_option! 3f41662
@jcf jcf Use let in favour of def in data attributes spec 5ed34ad
@jcf
jcf commented Jan 27, 2012

Hi @justinfrench,

Sorry it's taken me such a long time to sort out this PR. I've rebased master and resolved the outstanding issues (brought back the #warn_and_correct_option! method as you asked, and replaced a def with let in the test suite).

Please let me know if there's anything else you'd like changed and I'll hopefully be able to do it this evening.

All the best,

James

@justinfrench
Owner

@jcf it won't apply cleanly right now, but yeah, keen to get this into master now that we have a stable branch for 2.1

@felixbuenemann

Hmm, it seems to me, you didn't properly rebase your feature branch, because the pull request is referencing a lot of foreign commits.

Also I would advise you to split all of the bullet points you listed at the top into separate pull requests, unless there is a hard dependency between them, that you can't break by rebasing.

That way it will be much easier to discuss and merge the separate changes.

@jcf
jcf commented Feb 23, 2012

I used git rebase master, resolved conflicts as they arose, and git rebase --continued all the way to my final commit.

I don't have time to work on this PR at the moment, so will close it. If anyone wants to cherry-pick the commits, I'll keep my fork around.

@jcf jcf closed this Feb 23, 2012
@justinfrench justinfrench reopened this Feb 23, 2012
@justinfrench
Owner

Re-opened, so someone gets a reminder to do some cherry-picking :)

@felixbuenemann

I've cleanly rebased in https://github.com/fbuenemann/formtastic/tree/temp/jcf-data-attributes-pick-base and also squashed some fixups to make the diffs more readable. But I still feel this needs some splitting of existing commits and cherry piccking onto several branches to separate the changes.

Also I feel that some of the changes are questionable, eg. the addition of guard support and dependence on osx only gems. Because guard is a standalone binary it doesn't really need to be in the Gemfile.

@justinfrench
Owner

@fbuenemann if you're keen to get these split up and ready for pulling in, i'm keen, bu I think this one needs to be closed off

@felixbuenemann

@justinfrench I'm rather busy atm. I think the data attributes sugar is nice, so I'll take a look at it someday.

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