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

Types::Params::*.optional not handling empty string #419

Open
ermolaev opened this issue Feb 20, 2021 · 4 comments
Open

Types::Params::*.optional not handling empty string #419

ermolaev opened this issue Feb 20, 2021 · 4 comments

Comments

@ermolaev
Copy link

To Reproduce

Types::Params::Integer.optional[''] # => raise invalid value for Integer(): ""
Types::Optional::Params::Integer[''] # => nil

Types::Params::Date.optional[''] # => raise invalid date
Dry::Types["optional.params.date"] # => Nothing registered with the key "optional.params.date"

Expected behavior

Types::Params::Integer.optional[''] # => nil
Types::Params::Date.optional[''] # => nil

this behavior is confusing, I assumed that Types::Params::Date.optional equivalent Types::Params::Nil | Types::Params::Date

@flash-gordon
Copy link
Member

I can see how this can be confusing but I doubt we can fix it in any way. "Fixing" would be a breaking change first of all. Second, I don't really know how to fix it given how dry-types works internally, I can't see a good option (yet).
Adding more types to Optional::Params can be a solution of some sort, I think we can do it. And document that for params types there're subtleties in Optional:: vs .optional.

@solnic
Copy link
Member

solnic commented Feb 22, 2021

Oh I was aware of this and even had plans to address it before 1.0.0 😭 It somehow got under the radar! Don't you think this could be improved in 2.0.0?

@flash-gordon
Copy link
Member

I don't see a good way. Good in the sense that matches the internals. You can hack it but hacking things often brings more problems than it solves. Types don't "remember their origin", they don't belong to namespaces. If you change this then it has to be stored in the AST. You can probably keep it in meta then all ASTs will grow just to solve this particular problem. It'll be harder to define a type, it'll be harder to reason what type is. Do you have a better idea?

@solnic
Copy link
Member

solnic commented Feb 24, 2021

@flash-gordon right, you've made really good points! I'll give it some more thoughts...at some point...in the future. Maybe something to address in 3.0.0. I'd keep this one open for the time being and just label it as "unconfirmed" with 3.0.0 tentative milestone assigned...unless you're 100% sure it is not worth the effort and we should address it differently (maybe even remove .optional in 3.0.0)

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

3 participants