Apply constructor fn before checking validity of input

[cgeorgii]

This PR addresses the following incorrect behavior for Constructor#valid?, where a type with a constructor fn does not apply it to the value before checking its validity:

BMI = Dry::Types['strict.int'].constrained(gteq: 18, lteq: 42).constructor { |x| x.round }

BMI[17.5]
#=> 18

BMI.valid?(17.5)
#=> false

After the changes, the call to #valid? above will return true.

Since it's just a check, a rescue clause was added in order to always return a boolean.

[solnic]

This rescue clause is not a good idea, because it will be hiding bugs in code, so please remove

[cgeorgii]

Hey, thanks for the review.

My line of reasoning for this is as follows:

• The spec for the #valid method asserts that it returns a boolean.
• If the constructor block can't be applied to the input, the type is not valid, therefore returning false. In order for this to happen, I need to rescue the error.
• Letting the error bubble up makes sense when trying to construct the type but it shouldn't happen when checking whether it's valid or not.

I think this is a design decision that needs to be made and you know the project much better than I do. If you're certain that this is the way to go I will gladly change it, but I will need to change some of the specs as well because they will be broken.

[cgeorgii]

Maybe a cleaner option would be to have the #valid? method defined as such:

def valid?(value)
  constructed_value = fn[value]
rescue NoMethodError, TypeError
  false
else
  type.valid?(constructed_value)
end

This way we only rescue errors that happen when trying to apply the block to the value.

WDYT?

[solnic]

So, I think it's worth looking into all the places where valid? is used and making sure that this is what we want. I wouldn't just look at the spec because it doesn't give enough explanation why this method exists and how/when it's used.

[flash-gordon]

One thing to keep in mind. As of now, valid? is aliased ===. === in turn shouldn't raise exceptions, I'm pretty much sure...

[cgeorgii]

As far as I can tell after checking all occurrences in the repo, valid? is only used for control flow, so a boolean should be fine. I also checked dry-struct since it's based on dry-types but it doesn't use it either. If there's anything else worth checking, let me know.

Meanwhile, I'm going to push the cleaner version of the method as described above.

[cgeorgii]

@solnic Hi Piotr, is there anything else I can do regarding this PR? I also tested dry-validation and dry-struct using it and it was all green.

Let me know if I should rebase it as well.

[solnic]

@flash-gordon are you OK with merging this?