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

Type#fallback to mock previous implementation of Type#default #407

Closed
pyromaniac opened this issue Oct 29, 2020 · 10 comments
Closed

Type#fallback to mock previous implementation of Type#default #407

pyromaniac opened this issue Oct 29, 2020 · 10 comments
Labels

Comments

@pyromaniac
Copy link

pyromaniac commented Oct 29, 2020

Hey, thanks for a great gems first of all.

I was migrating from dry-types 0.14.0 and faced with the change of default behavior. There was even a type transformation proposed for dry-struct but I thought that maybe we can address such core functionality on a lower level.

I did the following patch:

Dry::Types::Builder.class_eval do
  # A ripoff of a `default` implementation,
  # except it is applied after coercion
  # and doesn't support blocks.
  def fallback(input)
    unless input.frozen?
      where = Dry::Core::Deprecations::STACK.()
      Dry::Core::Deprecations.warn(
        "#{input.inspect} is mutable."\
        " Be careful: types will return the same instance of the fallback"\
        " value every time. Call `.freeze` when setting the fallback."\
        "\n#{where}",
        tag: :"dry-types"
      )
    end

    if valid?(input)
      optional.append { |v| v.nil? ? input : v }.default(input)
    else
      raise Dry::Types::ConstraintError.new("fallback value #{input.inspect} violates constraints", input)
    end
  end
end

As you see it just appends a new constructor. But also makes the type default to be properly handled by dry-struct.
Also, I need to call optional first because the constructor gets simply replaced on Constructor types

Examples

Types::Params::Integer.fallback(0) gives us the same effect as was achieved with Types::Params::Integer.default(0) before.

We can think about the details of implementation but I believe this feature can be considered for the next version as it is useful.

@solnic
Copy link
Member

solnic commented Nov 11, 2020

@pyromaniac could you explain how the behavior of default actually changed so that it breaks for you now?

@pyromaniac
Copy link
Author

It didn't fallback to the default when nil was passed explicitly. In general, we lack value normalization in dry-types I believe. Something that adjust values after coercion. Normalization, for example, can be used to turn empty string to nil as well. This fallback proposal is simply a nerfed normalization.

@flash-gordon
Copy link
Member

I'm thinking about having Wrap types or something like that, it would be more general than constructors:

Types::Integer.wrap do |type, input, &fallback|
  type.(input, &fallback) + 1
end

Similar to how rack middleware works. I'm not quite sure it solves "all the problems" but it does add some flexibility. WDYT?

@pyromaniac
Copy link
Author

This would be actually perfect. I'll be able to build my own fallback type basing on this. Not sure if I got the interface right though. Why do we need those arguments? Wouldn't simple

Types::Integer.wrap do |value|
  value + 1
end

work? The same way as constructor works now, just happens after coercion.

@flash-gordon
Copy link
Member

The idea is you have full control over when the input gets coerced. I'm against adding post-constructors/normalization steps. We haven't had requests for this so far, this fact alone indicates we shouldn't rush into adding a specific solution. Wrapping types could be useful in a wider number of cases, kinda "by construction".

@pyromaniac
Copy link
Author

But then this Wrap type becomes a replacement of a Constructor type, it can do everything and more. Great point though.

@flash-gordon
Copy link
Member

But then this Wrap type becomes a replacement of a Constructor type

Yeah, in a way. Still, constructor will work for most cases. Besides, I'm not sure if this hypothetical "wrap" type will work, mmm, predictably everywhere. dry-types does have some surptising corner cases here and there regarding both usage and implementation. It's not a bad thing, though.

I'll play around with the idea in the free time, soon (as always).

@flash-gordon
Copy link
Member

Have a look at dry-rb/dry-schema#337, it looks like we're close to having nice support for this 🎉

@pyromaniac
Copy link
Author

pyromaniac commented Dec 27, 2020

@flash-gordon it looks awesome, thank you so much for working on this. I believe, DRY lacks this feature in general after default's behavior was changed.

@flash-gordon
Copy link
Member

It merged into master, at the moment I'm testing it in production (looks good!). Once I'm confident I'll cut a release. There are also a few other issues to fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants