Add the concept of NULL

[tristil]

Allow NULL to be referenced in formulas, and for passed-in variables to
have the value of 'null', which is evaluated as nil.

I would understand if you're not interested in this change. We need it because we want to have a function like this:

@calculator.add_function(:sum_values, :numeric, ->(*args) {
  args.reject! { |arg| arg.is_a?(String) }

  if args.none?
    ''
  else
    args.inject(BigDecimal('0.0')) do |sum, item|
      sum + (item || BigDecimal('0.0'))
    end
  end
})

... where the idea is that you only add up the values that are numeric. The reason that some charges are strings is because we are currently passing in the value of those variables as the string 'null'. This is because if the values of these variables was actually nil then the calculator would raise an unbound variable error before entering this function. And while this works, it's not great because we still have to pass back another string afterwards, requiring lots of blah != '' checks throughout our calculations.

[rubysolo]

Maybe it would be better to see if the context has the key identifier here, rather than checking the to see if the returned value is nil. That way, we could explicitly pass in nil as an identifier value.

[tristil]

That is definitely cleaner, and I agree that relying on a special string 'null' as an input is strange. The problem with the way things are written at the moment is that context has the unbound variable's key in it whether or not it was passed in as an input, because of the way that the context hash is built up here https://github.com/rubysolo/dentaku/blob/master/lib/dentaku/bulk_expression_solver.rb#L46 ... actually looking at that now, that looks like an avoidable behavior. Be right back.

[tristil]

Never mind, I don't see a way to avoid that behavior while keeping the recursive algorithm. So the behavior there always sets a key on the context.

It seems to me that one path forward is to build in checks for nullity into all the relevant operators, so that you would throw the unbound variable error when performing a +, -, / etc. That seems like a pain and complicates things, but I think it might have some benefits, like allowing an IF statement where the right side is irrelevant so it doesn't matter if its variable was passed or not. I won't work on that unless you think that is a good idea though.

[tristil]

@rubysolo Decided to go for something simpler for this PR. In the original code there were two parts within the commit:

1. The ability to say "NULL" in a formula, which was mentioned here IS Operator for IS BLANK or IS NOT BLANK  #18 and seems generally useful, e.g. IF(blah, 1, null)
2. The ability to pass in a variable as nil and have it be treated as NULL instead of an unbound variable

1. is easy but 2) is actually tricky to implement as things stand. I changed this commit to just handle 1) and I'd like to keep brainstorming on 2).

[tristil]

Came up with another commit for 2) which passes all tests. The change is a little subtle but I can't see anything wrong with it.

[rubysolo]

Thanks, this is a good addition! I took NULL out of the README, since it's not really an operator or function.

[tristil]

@rubysolo What do you think about releasing another gem for this change? It would help us with managing our dependencies since right now we have to point at the sha in multiple gemfiles instead of in a single gemspec.

[rubysolo]

Done!

[tristil]

Thanks :)

[muescha]

but the implemented NULL should be documented anyway in the doc