Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
DanRathbun opened this issue Sep 20, 2021 · 1 comment

Comments

@DanRathbun
Copy link
Contributor

DanRathbun commented Sep 20, 2021

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
Copy link
Contributor Author

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.)

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

No branches or pull requests

2 participants