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

Allow Param defaults to be an empty string? #76

Open
Rio517 opened this issue May 4, 2016 · 2 comments
Open

Allow Param defaults to be an empty string? #76

Rio517 opened this issue May 4, 2016 · 2 comments

Comments

@Rio517
Copy link

Rio517 commented May 4, 2016

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
Copy link
Owner

soveran commented May 17, 2016

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

Rio517 commented Sep 14, 2016

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

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