ohler55 / wabur

Web Application Builder using Ruby
MIT License
48 stars 3 forks source link

merge_map checking only prime_value #119

Closed ohaddahan closed 7 years ago

ohaddahan commented 7 years ago

While trying to run the sample app it failed during initialisation. I added the stack trace and some prints I added. It seems the issue is that the case condition checks prime_value and not other_value. Hence we stumble on a case where the + operator don't know how to concat Hash and Array.

merge_map prime.class = Hash
merge_map prime_value = String | other_value = String
merge_map prime_value = Hash | other_value = Hash
merge_map prime_value = {:dir=>"$BASE/data"} | other_value = {:dir=>"$BASE/examples/sample/wabur/data"}
merge_map prime.class = Hash
merge_map prime_value = String | other_value = String
merge_map prime_value = String | other_value = String
merge_map prime_value = Array | other_value = Hash
/Users/ohaddahan/Documents/Aptana Studio 3 Workspace/wabur_test/wabur/lib/wab/impl/configuration.rb:99:in `block in merge_map': no implicit conversion of Hash into Array (TypeError)
        from /Users/ohaddahan/Documents/Aptana Studio 3 Workspace/wabur_test/wabur/lib/wab/impl/configuration.rb:92:in `merge'
        from /Users/ohaddahan/Documents/Aptana Studio 3 Workspace/wabur_test/wabur/lib/wab/impl/configuration.rb:92:in `merge_map'
        from /Users/ohaddahan/Documents/Aptana Studio 3 Workspace/wabur_test/wabur/lib/wab/impl/configuration.rb:36:in `initialize'
        from /Users/ohaddahan/Documents/Aptana Studio 3 Workspace/wabur_test/wabur/bin/wabur:93:in `new'
        from /Users/ohaddahan/Documents/Aptana Studio 3 Workspace/wabur_test/wabur/bin/wabur:93:in `<top (required)>'
        from /Users/ohaddahan/.rvm/gems/ruby-2.4.2/bin/wabur:23:in `load'
        from /Users/ohaddahan/.rvm/gems/ruby-2.4.2/bin/wabur:23:in `<main>'
        from /Users/ohaddahan/.rvm/gems/ruby-2.4.2/bin/ruby_executable_hooks:15:in `eval'
        from /Users/ohaddahan/.rvm/gems/ruby-2.4.2/bin/ruby_executable_hooks:15:in `<main>'
# Recursive merge of other into prime.
      def merge_map(prime, other)
        puts "#{__method__} prime.class = #{prime.class.to_s}"
        prime.merge(other) { |key,prime_value,other_value|
          puts "#{__method__} prime_value = #{prime_value.class.to_s} | other_value = #{other_value.class.to_s}"
          case prime_value
          when Hash
            puts "#{__method__} prime_value = #{prime_value.inspect} | other_value = #{other_value.inspect}"
            merge_map(prime_value, other_value)
          when Array
      prime_value + other_value
          else
            other_value
          end
        }
ohaddahan commented 7 years ago

@ohler55 I'm not sure what is the desired result in this case.

ashmaroli commented 7 years ago

@ohaddahan Thank you for trying wabur. This particular class doesn't have unit tests defined, so there may be bugs hiding amidst. Either ways, can you repeat your trial with the following workaround change to Configuration#merge_map and get back to us?

  def merge_map(prime, other)
    prime.merge(other) { |key, prime_value, other_value|
      case prime_value
      when Hash
         merge_map(prime_value, other_value)
      when Array
        prime_value + Array(other_value)
      else
        other_value
      end
    }
  end

also, it'd be better if you could post your config file contents as well..

ashmaroli commented 7 years ago

@ohaddahan The bug has been traced back to commit a0526e0. I have created a temporary branch without that commit..

Feel free to continue testing WABuR by checking out branch temporary locally. Thank you for reporting this bug.

ohler55 commented 7 years ago

I'll look into it today. Thanks.

As for the desired behavior, maybe the correct behavior is to raise an exception as it indicates the configuration is wrong although a error message identifying the offending component would be better than the current one.

ohler55 commented 7 years ago

Sample configuration files are in examples/sample/wabur/wabur.conf and wabuse.json.

ohaddahan commented 7 years ago

Sorry for the delay , pulled and it confirmed it's working now.

ohler55 commented 7 years ago

Excellent.