The following construct is an ActiveSupport-ism:

  returning(Foo.new) do |foo|
    ...
  end

I don't especially like it, since it's both more verbose and less efficient
than the direct alternative:

  foo = Foo.new
  ...
  foo

It doesn't occur many times in Merb, so does anyone agree with me that it
should be removed?

I tried doing this (patch attached) and I find the code more readable
without. For example, in lib/merb/mixins/responder.rb

Before:

        def <=>(entry)
          returning((entry.quality <=> quality).to_s) do |c|
            c.replace((index <=> entry.index).to_s) if c == '0'
          end.to_i
        end

After:

        def <=>(entry)
          c = (entry.quality <=> quality).to_s
          c.replace((index <=> entry.index).to_s) if c == '0'
          c.to_i
        end

This second form also more clearly begs the question, why is the result from
<=> being converted to string and then back to integer? I don't know the
answer to that :-)

I believe the attached patch is OK - it doesn't break any specs. It doesn't
remove the definition of Object#returning itself, or its spec.

Regards,

Brian.
Index: lib/merb/gems.rb
===================================================================
--- lib/merb/gems.rb    (revision 508)
+++ lib/merb/gems.rb    (working copy)
@@ -27,16 +27,16 @@
       end
       
       def list_required(timing = :all)
-        returning({}) do |required|
-          with_manifest do |manifest|
-            manifest.each do |gem,options|
-              gem_when = (options["when"] || :after).to_sym
-              if timing == :all || gem_when == timing
-                required[gem] = options["version"]
-              end
+        required = {}
+        with_manifest do |manifest|
+          manifest.each do |gem,options|
+            gem_when = (options["when"] || :after).to_sym
+            if timing == :all || gem_when == timing
+              required[gem] = options["version"]
             end
           end
         end
+        required
       end
       
       def load_required(timing = :after)
Index: lib/merb/mixins/responder.rb
===================================================================
--- lib/merb/mixins/responder.rb        (revision 508)
+++ lib/merb/mixins/responder.rb        (working copy)
@@ -65,18 +65,18 @@
           
           def self.parse(accept_header)
             # parse the raw accept header into a unique, sorted array of 
AcceptType objects
-            returning( accept_header.split(/,/).enum_for(:each_with_index).map 
do |entry,index|
+            list = accept_header.split(/,/).enum_for(:each_with_index).map do 
|entry,index|
               AcceptType.new(entry,index += 1)
-            end.sort.uniq ) do |list|
-              # firefox (and possibly other browsers) send broken default 
accept headers.
-              # fix them up by sorting alternate xml forms (namely 
application/xhtml+xml)
-              # ahead of pure xml types (application/xml,text/xml).
-              if app_xml = list.detect{|e| e.super_range == 'application/xml'}
-                list.select{|e| e.to_s =~ /\+xml/}.each { |acc_type|
-                  list[list.index(acc_type)],list[list.index(app_xml)] = 
-                    list[list.index(app_xml)],list[list.index(acc_type)] }
-              end
+            end.sort.uniq
+            # firefox (and possibly other browsers) send broken default accept 
headers.
+            # fix them up by sorting alternate xml forms (namely 
application/xhtml+xml)
+            # ahead of pure xml types (application/xml,text/xml).
+            if app_xml = list.detect{|e| e.super_range == 'application/xml'}
+              list.select{|e| e.to_s =~ /\+xml/}.each { |acc_type|
+                list[list.index(acc_type)],list[list.index(app_xml)] = 
+                  list[list.index(app_xml)],list[list.index(acc_type)] }
             end
+            list
           end
           
         private
@@ -128,9 +128,9 @@
         end
       
         def <=>(entry)
-          returning((entry.quality <=> quality).to_s) do |c|
-            c.replace((index <=> entry.index).to_s) if c == '0'
-          end.to_i
+          c = (entry.quality <=> quality).to_s
+          c.replace((index <=> entry.index).to_s) if c == '0'
+          c.to_i
         end
       
         def eql?(entry)
Index: lib/merb/template/erubis.rb
===================================================================
--- lib/merb/template/erubis.rb (revision 508)
+++ lib/merb/template/erubis.rb (working copy)
@@ -40,12 +40,12 @@
             return @@erbs[path]
           else  
             begin
-              returning ::Erubis::MEruby.new(IO.read(path)) do |eruby|
-                eruby.init_evaluator :filename => path
-                if cache_template?(path)
-                  @@erbs[path], @@mtimes[path] = eruby, Time.now
-                end  
-              end
+              eruby = ::Erubis::MEruby.new(IO.read(path))
+              eruby.init_evaluator :filename => path
+              if cache_template?(path)
+                @@erbs[path], @@mtimes[path] = eruby, Time.now
+              end  
+              eruby
             rescue Errno::ENOENT
               raise "No template found at path: #{path}"
             end
Index: lib/merb/mailer.rb
===================================================================
--- lib/merb/mailer.rb  (revision 508)
+++ lib/merb/mailer.rb  (working copy)
@@ -52,9 +52,9 @@
     def initialize(o={})
       self.config = :sendmail if config.nil?
       o[:rawhtml] = o.delete(:html)
-      @mail = returning MailFactory.new() do |m|
-        o.each { |k,v| m.send "#{k}=", v }
-      end
+      m = MailFactory.new()
+      o.each { |k,v| m.send "#{k}=", v }
+      @mail = m
     end
     
   end
_______________________________________________
Merb-devel mailing list
[email protected]
http://rubyforge.org/mailman/listinfo/merb-devel

Reply via email to