ruby-core

Hi,

At Mon, 11 Sep 2006 15:03:42 +0900,
greg weber wrote in [ruby-core:08834]:
> This is my first patch submission, so I hope I am not being rude, and
> I appreciate the your response and the work that everyone here does.

Don't mind.

> I think it is important to question anything that conflict with your
> ideas about the architecture.  However, a change that conflicts with
> an architectural idea does not in itself mean the change is bad.  It
> could mean that the architectural idea is overly rigid.

Agree.

> My patch does seem kind of hackish.  Perhaps you would be more
> comfortable with adding a state variable, @auto_options = {}, that
> would implement this option hash instead of using self.  I don't see
> any downside to this- users are free to ignore it.  Those who use it
> will avoid creating a boilerplate outer class and boilerplate variable
> settings, making them more productive and happier :)

Adding a state variable also means to change the parser.  Still I'm
not feel nice about it.  Instead, how do you feel about improving
OptionParser#getopts method?

> I do not see the exclusive and requires methods.  What version do I
> need for these to work?

It was imaginary example.  No implementation yet.

> One more comment- I think the OptionParser library would do well to
> make some of its public methods private or combine them.  The
> multitude of public methods maked it harder to figure out which
> methods should be used.  For example, it would perhaps be better to
> eliminate on and just use define.  It would also be good to make order
> and permute private, but callable from parse by setting keyword
> arguments.

Agreed.  It was originated from the analogy of case/when/end first,
but I admit that the name "on" was too wrong.

> One more question- is there a way to collect the help output message
> from OptionParser with RDoc?

Ah, optparse is often considered too slow already :).

-- 
Nobu Nakada

Attachments (2)

optparse_getopts-1.8.diff (3.32 KB, text/x-diff)

Index: lib/optparse.rb
===================================================================
RCS file: /home/K8052/cvs/ruby/lib/optparse.rb,v
retrieving revision 1.40.2.12
diff -U 2 -p -r1.40.2.12 optparse.rb
--- lib/optparse.rb	4 Aug 2006 22:00:21 -0000	1.40.2.12
+++ lib/optparse.rb	11 Sep 2006 06:22:48 -0000
@@ -347,14 +347,10 @@ class OptionParser
     #
     def conv_arg(arg, val = nil)
-      if block
-        if conv
-          val = conv.call(*val)
-        else
-          val = *val
-        end
-        return arg, block, val
+      if conv
+        val = conv.call(*val)
       else
-        return arg, nil
+        val = *val
       end
+      return arg, block, val
     end
     private :conv_arg
@@ -404,4 +400,11 @@ class OptionParser
 
     #
+    # Main name of the switch.
+    #
+    def switch_name
+      (long.first || short.first).sub(/\A-+(?:\[no-\])?/, '').intern
+    end
+
+    #
     # Switch that takes no arguments.
     #
@@ -1200,5 +1203,10 @@ class OptionParser
   # Same as #order, but removes switches destructively.
   #
-  def order!(argv = default_argv, &nonopt)
+  def order!(argv = default_argv, &block)
+    parse_in_order(argv, &block)
+  end
+
+  # :nodoc:
+  def parse_in_order(argv = default_argv, setter = nil, &nonopt)
     opt, arg, sw, val, rest = nil
     nonopt ||= proc {|arg| throw :terminate, arg}
@@ -1215,6 +1223,7 @@ class OptionParser
           end
           begin
-            opt, sw, val = sw.parse(rest, argv) {|*exc| raise(*exc)}
-            sw.call(val) if sw
+            opt, cb, *val = sw.parse(rest, argv) {|*exc| raise(*exc)}
+            val = cb.call(*val) if cb
+            setter.call(sw.switch_name, val) if setter
           rescue ParseError
             raise $!.set_option(arg, rest)
@@ -1242,8 +1251,9 @@ class OptionParser
           end
           begin
-            opt, sw, val = sw.parse(val, argv) {|*exc| raise(*exc) if eq}
+            opt, cb, val = sw.parse(val, argv) {|*exc| raise(*exc) if eq}
             raise InvalidOption, arg if has_arg and !eq and arg == "-#{opt}"
             argv.unshift(opt) if opt and (opt = opt.sub(/\A-*/, '-')) != '-'
-            sw.call(val) if sw
+            val = cb.call(val) if cb
+            setter.call(sw.switch_name, val) if setter
           rescue ParseError
             raise $!.set_option(arg, arg.length > 2)
@@ -1261,4 +1271,5 @@ class OptionParser
     argv
   end
+  private :parse_in_order
 
   #
@@ -1305,5 +1316,5 @@ class OptionParser
   # Wrapper method for getopts.rb.
   #
-  def getopts(argv, single_options, *long_options)
+  def getopts(argv, single_options = nil, *long_options)
     result = {}
 
@@ -1311,8 +1322,8 @@ class OptionParser
       if val
         result[opt] = nil
-        define("-#{opt} VAL") {|val| result[opt] = val}
+        define("-#{opt} VAL")
       else
         result[opt] = false
-        define("-#{opt}") {result[opt] = true}
+        define("-#{opt}")
       end
     end if single_options
@@ -1322,12 +1333,12 @@ class OptionParser
       if val
         result[opt] = val.empty? ? nil : val
-        define("--#{opt} VAL") {|val| result[opt] = val}
+        define("--#{opt} VAL")
       else
         result[opt] = false
-        define("--#{opt}") {result[opt] = true}
+        define("--#{opt}")
       end
     end
 
-    order!(argv)
+    parse_in_order(argv, result.method(:[]=))
     result
   end

optparse_getopts-1.9.diff (3.13 KB, text/x-diff)

Index: lib/optparse.rb
===================================================================
RCS file: /home/K8052/cvs/ruby/lib/optparse.rb,v
retrieving revision 1.66
diff -U 2 -p -r1.66 optparse.rb
--- lib/optparse.rb	10 Sep 2006 00:20:03 -0000	1.66
+++ lib/optparse.rb	11 Sep 2006 06:22:50 -0000
@@ -347,14 +347,10 @@ class OptionParser
     #
     def conv_arg(arg, val = nil)
-      if block
-        if conv
-          val = conv.yield(val)
-        else
-          val = *val
-        end
-        return arg, block, val
+      if conv
+        val = conv.yield(val)
       else
-        return arg, nil
+        val = *val
       end
+      return arg, block, val
     end
     private :conv_arg
@@ -416,4 +412,11 @@ class OptionParser
 
     #
+    # Main name of the switch.
+    #
+    def switch_name
+      (long.first || short.first).sub(/\A-+(?:\[no-\])?/, '').intern
+    end
+
+    #
     # Switch that takes no arguments.
     #
@@ -1237,4 +1240,9 @@ class OptionParser
   #
   def order!(argv = default_argv, &nonopt)
+    parse_in_order(argv, &block)
+  end
+
+  # :nodoc:
+  def parse_in_order(argv = default_argv, setter = nil, &nonopt)
     opt, arg, sw, val, rest = nil
     nonopt ||= proc {|arg| throw :terminate, arg}
@@ -1251,6 +1259,7 @@ class OptionParser
           end
           begin
-            opt, sw, *val = sw.parse(rest, argv) {|*exc| raise(*exc)}
-            sw.call(*val) if sw
+            opt, cb, *val = sw.parse(rest, argv) {|*exc| raise(*exc)}
+            val = cb.call(*val) if cb
+            setter.call(sw.switch_name, val) if setter
           rescue ParseError
             raise $!.set_option(arg, rest)
@@ -1279,8 +1288,9 @@ class OptionParser
           end
           begin
-            opt, sw, *val = sw.parse(val, argv) {|*exc| raise(*exc) if eq}
+            opt, cb, val = sw.parse(val, argv) {|*exc| raise(*exc) if eq}
             raise InvalidOption, arg if has_arg and !eq and arg == "-#{opt}"
             argv.unshift(opt) if opt and (opt = opt.sub(/\A-*/, '-')) != '-'
-            sw.call(*val) if sw
+            val = cb.call(*val) if cb
+            setter.call(sw.switch_name, val) if setter
           rescue ParseError
             raise $!.set_option(arg, arg.length > 2)
@@ -1349,5 +1359,5 @@ class OptionParser
   # Wrapper method for getopts.rb.
   #
-  def getopts(argv, single_options, *long_options)
+  def getopts(argv, single_options = nil, *long_options)
     result = {}
 
@@ -1355,8 +1365,8 @@ class OptionParser
       if val
         result[opt] = nil
-        define("-#{opt} VAL") {|val| result[opt] = val}
+        define("-#{opt} VAL")
       else
         result[opt] = false
-        define("-#{opt}") {result[opt] = true}
+        define("-#{opt}")
       end
     end if single_options
@@ -1366,12 +1376,12 @@ class OptionParser
       if val
         result[opt] = val.empty? ? nil : val
-        define("--#{opt} VAL") {|val| result[opt] = val}
+        define("--#{opt} VAL")
       else
         result[opt] = false
-        define("--#{opt}") {result[opt] = true}
+        define("--#{opt}")
       end
     end
 
-    order!(argv)
+    parse_in_order(argv, result.method(:[]=))
     result
   end

Thread

Prev Next

In This Thread

Prev Next