public
Description: Ruby on Rails
Homepage: http://rubyonrails.org
Clone URL: git://github.com/rails/rails.git
Make assert_select_rjs code more readable, make use of unused constants and use 
more simple Regexps.

[#540 state:resolved]

Signed-off-by: Jeremy Kemper <jeremy@bitsweat.net>
miloops (author)
Thu Aug 07 09:07:53 -0700 2008
jeremy (committer)
Fri Aug 29 17:52:26 -0700 2008
commit  11eb29f60ab79caf22f7ca715500e32d9a1b03a2
tree    336a2bc66ca6916237d3df51fcd5cce976431c7e
parent  6450d6ca76603bc5d1b1a6cdcb77e33e35f53d83
...
396
397
398
399
400
 
 
401
402
403
404
405
406
407
 
408
409
410
411
412
 
 
 
413
414
415
416
417
418
419
420
421
422
423
424
425
426
427
428
429
430
431
432
433
434
435
436
437
438
439
440
441
442
443
444
445
446
 
 
 
 
447
448
449
...
588
589
590
591
592
593
594
595
596
597
598
599
 
 
 
 
 
 
 
600
601
602
603
 
 
 
 
604
605
 
606
607
608
609
610
 
 
611
612
613
...
621
622
623
624
625
 
 
626
627
628
...
396
397
398
 
 
399
400
401
402
403
404
 
 
 
405
406
 
 
 
 
407
408
409
410
411
412
413
414
 
415
416
417
 
418
419
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
420
421
422
423
424
425
426
...
565
566
567
 
 
 
 
 
 
 
 
 
568
569
570
571
572
573
574
575
 
 
 
576
577
578
579
580
 
581
582
 
 
 
 
583
584
585
586
587
...
595
596
597
 
 
598
599
600
601
602
0
@@ -396,54 +396,31 @@ module ActionController
0
       #   # The same, but shorter.
0
       #   assert_select "ol>li", 4
0
       def assert_select_rjs(*args, &block)
0
-        rjs_type = nil
0
-        arg      = args.shift
0
+        rjs_type = args.first.is_a?(Symbol) ? args.shift : nil
0
+        id       = args.first.is_a?(String) ? args.shift : nil
0
 
0
         # If the first argument is a symbol, it's the type of RJS statement we're looking
0
         # for (update, replace, insertion, etc). Otherwise, we're looking for just about
0
         # any RJS statement.
0
-        if arg.is_a?(Symbol)
0
-          rjs_type = arg
0
-
0
+        if rjs_type
0
           if rjs_type == :insert
0
-            arg = args.shift
0
-            position  = arg
0
-            insertion = "insert_#{arg}".to_sym
0
-            raise ArgumentError, "Unknown RJS insertion type #{arg}" unless RJS_STATEMENTS[insertion]
0
+            position  = args.shift
0
+            insertion = "insert_#{position}".to_sym
0
+            raise ArgumentError, "Unknown RJS insertion type #{position}" unless RJS_STATEMENTS[insertion]
0
             statement = "(#{RJS_STATEMENTS[insertion]})"
0
           else
0
             raise ArgumentError, "Unknown RJS statement type #{rjs_type}" unless RJS_STATEMENTS[rjs_type]
0
             statement = "(#{RJS_STATEMENTS[rjs_type]})"
0
           end
0
-          arg = args.shift
0
         else
0
           statement = "#{RJS_STATEMENTS[:any]}"
0
         end
0
-        position ||= Regexp.new(RJS_INSERTIONS.join('|'))
0
 
0
         # Next argument we're looking for is the element identifier. If missing, we pick
0
-        # any element.
0
-        if arg.is_a?(String)
0
-          id = Regexp.quote(arg)
0
-          arg = args.shift
0
-        else
0
-          id = "[^\"]*"
0
-        end
0
-
0
-        pattern =
0
-          case rjs_type
0
-            when :chained_replace, :chained_replace_html
0
-              Regexp.new("\\$\\(\"#{id}\"\\)#{statement}\\(#{RJS_PATTERN_HTML}\\)", Regexp::MULTILINE)
0
-            when :remove, :show, :hide, :toggle
0
-              Regexp.new("#{statement}\\(\"#{id}\"\\)")
0
-            when :replace, :replace_html
0
-              Regexp.new("#{statement}\\(\"#{id}\", #{RJS_PATTERN_HTML}\\)")
0
-            when :insert, :insert_html
0
-              Regexp.new("Element.insert\\(\\\"#{id}\\\", \\{ #{position}: #{RJS_PATTERN_HTML} \\}\\);")
0
-             else
0
-              Regexp.union(Regexp.new("#{statement}\\(\"#{id}\", #{RJS_PATTERN_HTML}\\)"),
0
-                Regexp.new("Element.insert\\(\\\"#{id}\\\", \\{ #{position}: #{RJS_PATTERN_HTML} \\}\\);"))
0
-           end
0
+        # any element, otherwise we replace it in the statement.
0
+        pattern = Regexp.new(
0
+          id ? statement.gsub(RJS_ANY_ID, "\"#{id}\"") : statement
0
+        )
0
 
0
         # Duplicate the body since the next step involves destroying it.
0
         matches = nil
0
@@ -588,26 +565,23 @@ module ActionController
0
 
0
       protected
0
         unless const_defined?(:RJS_STATEMENTS)
0
-          RJS_STATEMENTS = {
0
-            :replace              => /Element\.replace/,
0
-            :replace_html         => /Element\.update/,
0
-            :chained_replace      => /\.replace/,
0
-            :chained_replace_html => /\.update/,
0
-            :remove               => /Element\.remove/,
0
-            :show                 => /Element\.show/,
0
-            :hide                 => /Element\.hide/,
0
-            :toggle                 => /Element\.toggle/
0
+          RJS_PATTERN_HTML  = "\"((\\\\\"|[^\"])*)\""
0
+          RJS_ANY_ID        = "\"([^\"])*\""
0
+          RJS_STATEMENTS    = {
0
+            :chained_replace      => "\\$\\(#{RJS_ANY_ID}\\)\\.replace\\(#{RJS_PATTERN_HTML}\\)",
0
+            :chained_replace_html => "\\$\\(#{RJS_ANY_ID}\\)\\.update\\(#{RJS_PATTERN_HTML}\\)",
0
+            :replace_html         => "Element\\.update\\(#{RJS_ANY_ID}, #{RJS_PATTERN_HTML}\\)",
0
+            :replace              => "Element\\.replace\\(#{RJS_ANY_ID}, #{RJS_PATTERN_HTML}\\)"
0
           }
0
-          RJS_STATEMENTS[:any] = Regexp.new("(#{RJS_STATEMENTS.values.join('|')})")
0
-          RJS_PATTERN_HTML = /"((\\"|[^"])*)"/
0
-          RJS_INSERTIONS = [:top, :bottom, :before, :after]
0
+          [:remove, :show, :hide, :toggle].each do |action|
0
+            RJS_STATEMENTS[action] = "Element\\.#{action}\\(#{RJS_ANY_ID}\\)"
0
+          end
0
+          RJS_INSERTIONS = ["top", "bottom", "before", "after"]
0
           RJS_INSERTIONS.each do |insertion|
0
-            RJS_STATEMENTS["insert_#{insertion}".to_sym] = /Element.insert\(\"([^\"]*)\", \{ #{insertion.to_s.downcase}: #{RJS_PATTERN_HTML} \}\);/
0
+            RJS_STATEMENTS["insert_#{insertion}".to_sym] = "Element.insert\\(#{RJS_ANY_ID}, \\{ #{insertion}: #{RJS_PATTERN_HTML} \\}\\)"
0
           end
0
-          RJS_STATEMENTS[:insert_html] = Regexp.new(RJS_INSERTIONS.collect do |insertion|
0
-            /Element.insert\(\"([^\"]*)\", \{ #{insertion.to_s.downcase}: #{RJS_PATTERN_HTML} \}\);/
0
-          end.join('|'))
0
-          RJS_PATTERN_EVERYTHING = Regexp.new("#{RJS_STATEMENTS[:any]}\\(\"([^\"]*)\", #{RJS_PATTERN_HTML}\\)", Regexp::MULTILINE)
0
+          RJS_STATEMENTS[:insert_html] = "Element.insert\\(#{RJS_ANY_ID}, \\{ (#{RJS_INSERTIONS.join('|')}): #{RJS_PATTERN_HTML} \\}\\)"
0
+          RJS_STATEMENTS[:any] = Regexp.new("(#{RJS_STATEMENTS.values.join('|')})")
0
           RJS_PATTERN_UNICODE_ESCAPED_CHAR = /\\u([0-9a-zA-Z]{4})/
0
         end
0
 
0
@@ -621,8 +595,8 @@ module ActionController
0
             root = HTML::Node.new(nil)
0
 
0
             while true
0
-              next if body.sub!(RJS_PATTERN_EVERYTHING) do |match|
0
-                html = unescape_rjs($3)
0
+              next if body.sub!(RJS_STATEMENTS[:any]) do |match|
0
+                html = unescape_rjs(match)
0
                 matches = HTML::Document.new(html).root.children.select { |n| n.tag? }
0
                 root.children.concat matches
0
                 ""
...
599
600
601
 
 
 
 
 
602
603
604
...
599
600
601
602
603
604
605
606
607
608
609
0
@@ -599,6 +599,11 @@ class AssertSelectTest < Test::Unit::TestCase
0
     end
0
   end
0
 
0
+  def test_assert_select_rjs_raise_errors
0
+    assert_raises(ArgumentError) { assert_select_rjs(:destroy) }
0
+    assert_raises(ArgumentError) { assert_select_rjs(:insert, :left) }
0
+  end
0
+
0
   # Simple selection from a single result.
0
   def test_nested_assert_select_rjs_with_single_result
0
     render_rjs do |page|

Comments

dchelimsky Sat Sep 06 04:50:36 -0700 2008

The following test fails as of this commit:

  def test_assert_select_rjs_for_positioned_insert_should_fail_when_mixing_arguments
    render_rjs do |page|
      page.insert_html :top, "test1", "<div id=\"1\">foo</div>" 
      page.insert_html :bottom, "test2", "<div id=\"2\">foo</div>" 
    end
    assert_raises {assert_select_rjs :insert, :top, "test2"}
  end

See http://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/982 for more info.

greatseth Sat Sep 06 07:06:13 -0700 2008

fwiw, if we’re talking about making code more readable, I don’t understand why people ever have ternary expressions that can return nil.

args.shift if args.first.is_a?(Symbol)

seems better for readability than

args.first.is_a?(Symbol) ? args.shift : nil

greatseth Sat Sep 06 07:09:53 -0700 2008

granted, you’d have to do it like

foo = (bar if baz)

for the example here, which is maybe a little weird. I think I was thinking of situations where I did this behavior when assigning ivars, which of course relies on their defaulting to nil anyway.

so probably nevermind my ill conceived comment.

sheepish