SketchUp / htmldialog-inputbox

UI::HtmlDialog example recreating UI.inputbox functionality in the SketchUp Ruby API
MIT License
6 stars 5 forks source link

Options hash defaults getting wiped (to `nil`) when named arguments are omitted. #11

Open DanRathbun opened 3 years ago

DanRathbun commented 3 years ago

Options hash defaults getting wiped (to nil) when named arguments are omitted.

There are 2 factors contributing:

(1) ArgumentParser#get_options_hash() is wiping out the defaults that are set in HtmlUI::Inputbox#initialize() when named arguments are omitted.

Currently in ArgumentParser#get_options_hash():

    def get_options_hash(options)
      options = {
        title: options[:title],
        accept_button: options[:accept_button],
        cancel_button: options[:cancel_button],
        inputs: options[:inputs].map(&:as_json),
      }
    end

What is happening is a brute force merge! with a modification replacement of the :inputs value.

When a named key/value pair is omitted in the argument hash, then calling options[:keyname] will return nil. So this pattern is forcing the existence of each of the named keys with the omitted keys set to a nil value.

(2) In HtmlUI::Inputbox#initialize() we have ...

        @options = defaults.merge(options)

Hash#merge does not handle nil values any differently than any other class value. It simply replaces any value in the receiver hash, whose key exists in the argument hash, even if the new value is nil.

So, the @options then has valid values from defaults set to nil.


Two things can be done ...

Use a block with Hash#merge in HtmlUI::Inputbox#initialize(). The block is only called if duplicate keys exist in each hash. Non-duplicates in receiver hash are brought forward unchanged. Any extra keys in argument hash are brought forward unchanged.

      def initialize(*args)
        omit = caller_locations(2)[0].base_label == 'inputbox' ? 2 : 1
        callstack = caller(omit)
        fail(ArgumentError,'No arguments given. (1..4 expected)',callstack) if args.empty?
        defaults = {
          title: Sketchup.app_name,
          accept_button: 'Accept',
          cancel_button: 'Cancel',
          inputs: [],
        }
        options = ArgumentParser.new(callstack).parse(*args)
        @options = defaults.merge(options) do |key, oldval, newval|
          !newval.nil? ? newval : oldval
        end
      end

And (2), in ArgumentParser ...

  class ArgumentParser

    def initialize(callstack, *args)
      @callstack = callstack
    end

    def get_options_hash(options)
      if !options.key?(:inputs)
        fail(ArgumentError,'inputs array argument is required.',@callstack)
      elsif options[:inputs].empty?
        fail(ArgumentError,'inputs array argument is empty.',@callstack)
      end
      # Map :inputs Array members into property Hashes:
      options[:inputs].map!(&:as_json)
      #
      options
    end

NOTE: The @callstack should also be used when raising other exceptions from any of the other ArgumentParser methods. Normally in a library class we just use the [Kernel#caller]() with it's default of omitting 1 item from the callstack, so that the consumer sees where they have made the error in calling the library at the top of the callstack.

But this library is complicated because it has a chain of nested method calls that originate from both a class constructor and a class wrapper that calls that constructor. (Hence the test of where the HtmlUI::Inputbox#initialize() method was called from.)

IMO, having argument parsing as a separate class is confusing and complicates the library. I also think it doesn't teach good coding. It does not encapsulate any state, ie, needs no instance variables (ignoring my addition of @callstack.) It only has processing code that receives and passes all data as method arguments. Basically it acts like a mixin module, and actually could be better used as one mixed into the HtmlUI::Inputbox class if a separate code object is really needed. (But class definitions can span more than one physical file.)

In my edition, I'm going to move the conditional expression in parse() into HtmlUI::Inputbox#initialize() and then the other methods into the private section of the HtmlUI::Inputbox class. (I might leave it as a separate file.)

DanRathbun commented 3 years ago

NOTE: I deleted the "Aside" question in the 1st post as it came from a misreading of the code, so was invalid. (ie, #as_json is a hash mapping method, not a serialization method. Mea culpa.)