soveran / cuba

Rum based microframework for web development.
http://cuba.is
MIT License
1.44k stars 249 forks source link

Allow Param defaults to be an empty string? #76

Open Rio517 opened 8 years ago

Rio517 commented 8 years ago

Hi,

Would make sense to allow empty strings as a default param value? While it makes sense to prevent empty strings from being captured from a request, the current implementation discards both empty params and empty defaults as they pass through the control flow. See: https://github.com/soveran/cuba/blob/master/lib/cuba.rb#L264

Would you be open to a pull request addressing this or would you guys be interested in tackling this small change?

Background Scenario/Usecase

I have a PORO JSON presenter/serializer wrapping an instance of data object, which together return validation errors. I'd like Cuba to pass the empty strings to my presenter/object, and could do so if I could set empty strings as my default. Currently, I'd have to have a second on true block that I'd rather not have cluttering up my code.

soveran commented 8 years ago

Hello @Rio517, I have this implementation in mind:

  def param(key, default = nil)
    value = req[key].to_s

    lambda do
      if value.empty?
        captures << default if default
      else
        captures << value
      end
    end
  end

I think it works for your use case. The only problem is that it could break some applications that use "" as a default value and expect the match to return false. My guess is that nobody is using such default value, but we should assume we are breaking compatibility. With that implementation in mind, we can explore three things:

  1. Is the new behavior correct?
  2. Is there a way to solve the problem without modifying param?
  3. Is there a better implementation?

Right now I tend to think the new behavior is less surprising, and it's fine if we don't find a better implementation right away as long as this one is clear enough and correct. What do you think?

Rio517 commented 8 years ago

Hi,

Sorry for delayed response. Yes. I think this new behavior is correct, and I'm not sure how one could solve this without modifying param method.

Finally, I would strongly agree that this approach is the least surprising.

Thanks,

-Mario