dry-rb / dry-types

Flexible type system for Ruby with coercions and constraints
https://dry-rb.org/gems/dry-types
MIT License
861 stars 134 forks source link

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

Closed pyromaniac closed 3 years ago

pyromaniac commented 4 years ago

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 commented 4 years ago

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

pyromaniac commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

flash-gordon commented 4 years ago

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 commented 3 years ago

Have a look at https://github.com/dry-rb/dry-schema/pull/337, it looks like we're close to having nice support for this 🎉

pyromaniac commented 3 years ago

@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 commented 3 years ago

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.