Validate input type and return a descriptive error

[bryszard]

As described in issue https://github.com/dry-rb/dry-validation/issues/716, the error raised when wrong input is given to the Contract#call method is confusing. It is the NoMethodError instead of ArgumentError.

I'm adding here a validation to fix that and return a descriptive error message.

If only we had a type system... ;)

[esparta]

IMO the new error is more descriptive but sadly is one of the most "hot" path of the whole project as in this might slow down normal operation by checking something it hasn't been checking all those years.

[bryszard]

Well, I think we have 2 solutions here - close the issue without solving or slowing down this path by this bit. Do you see some third way?

[esparta]

In order to have an informed decision perhaps the maintainers would need a benchmark where we document this decision.

Do you see some third way?

Maybe document this known issue if the decision (after review benchmarks) is not change the error.

[bryszard]

@esparta You're right, benchmark is the way to take the informed decision. So I did it.

Methodology

Use the test contract same as defined in call_spec (here). On that run in the console:

require "benchmark"

n = 100_000
Benchmark.bm do |x|
  x.report { n.times { contract.(email: "john@doe.org", age: 19) } }
end

Run benchmark 5 times and choose minimum and maximum result before the change and after the change.

Results

Before the change

MIN 
user     system      total        real
8.305380   0.025480   8.330860 (  8.333271)

MAX (first call)
user     system      total        real
8.542934   0.044865   8.587799 (  8.696112)

After the change

MIN 
user     system      total        real
8.305227   0.021869   8.327096 (  8.328431)

MAX (first call)
user     system      total        real
8.566376   0.047062   8.613438 (  8.700889)

Conclusion

Differences in benchmarks are very small, I'd say they fit in the error's margin.

---

So what do you think? Is it good enough?

[bryszard]

Looks like we cannot really run the tests on version 2.7 anymore? Because of the dependency on the dry-monads? - https://github.com/dry-rb/dry-validation/actions/runs/3824133459/jobs/6512939739 Should we create another PR to remove it?

EDIT: Created https://github.com/dry-rb/dry-validation/pull/725, waiting until it's merged, so I can fix the CI here.

[solnic]

Looks like we cannot really run the tests on version 2.7 anymore? Because of the dependency on the dry-monads?

@flash-gordon we need to revert this, it's a bit too early to drop 2.7 in dry-rb

[bryszard]

Rebased the branch after Ruby 2.7 support was re-added.

[bryszard]

The CI for Ruby 2.7 is still fails because of dry-monads being dependent on Ruby >= 3.0. Need to wait for the change there.

[flash-gordon]

something's off with syncing gemspec files across gems using repobot

[flash-gordon]

@solnic does it get triggered when I edit project.yml?

[solnic]

@flash-gordon yes but only in main branches

[flash-gordon]

@solnic good, and what about when I edit gemspec.erb, does it get synced? It's a bit tricky. Asking because I probably edited it in template-gem and didn't see changes in gemspec files

[solnic]

@solnic good, and what about when I edit gemspec.erb, does it get synced? It's a bit tricky. Asking because I probably edited it in template-gem and didn't see changes in gemspec files

Yes it should because it's configured as a template file too.

[flash-gordon]

@solnic then unless I've missed something it's broken. This commit https://github.com/dry-rb/template-gem/commit/6052746a3ad434c6cf754ce49e9eaae5a5c6a34a didn't trigger a sync in dry-validation.

[solnic]

@flash-gordon uhm OK, I'll look into it

[solnic]

@flash-gordon FYI your commit didn't trigger gemspec sync because it did not have any effect on the rendered gemspec files. I just changed 2.7.0 to 2.7 in the template and it did trigger updates as it did change the actual rendered gemspec files.

Can this now be rebased on top of latest main? The build should pass.

[flash-gordon]

@solnic that makes sense 😅