search

codegram / date_validator

A simple, ORM agnostic, Ruby >=2.2 compatible date validator for Rails, based on ActiveModel.
http://thoughts.codegram.com/date-validation-with-rails-3
MIT License
496 stars 82 forks source link

Spread keyword args to support Ruby 3 #85

Closed ball-hayden closed 3 years ago

ball-hayden commented 3 years ago

✍️ Description

As of Rails 6.1, ActiveRecord::Errors.add spreads keyword arguments rather than accepting an options hash (https://api.rubyonrails.org/v6.1.0/classes/ActiveModel/Errors.html#method-i-add).

In Ruby 3, there is no implicit cast between a last hash argument and explicit keyword arguments, causing argument errors:

     ArgumentError:
       wrong number of arguments (given 3, expected 1..2)
     # ./vendor/bundle/ruby/3.0.0/gems/activemodel-6.1.0.rc2/lib/active_model/errors.rb:404:in `add'
     # ./vendor/bundle/ruby/3.0.0/gems/date_validator-0.10.0/lib/active_model/validations/date_validator.rb:97:in `block in validate_each'

✅ QA

Before requesting a review, please make sure that:

ball-hayden commented 3 years ago

~https://github.com/actions/setup-ruby doesn't support Ruby 3 yet, so I can't add Ruby 3 to CI.~

ball-hayden commented 3 years ago

The test failure (2.7, 5.2) appears to be a flake, but I can't see a way to re-run the check.

I can reproduce the flake with the specified seed (it passes with other seeds), but I can't trace the cause 😦

mrcasals commented 3 years ago

Hi @ball-hayden! We're using https://github.com/ruby/setup-ruby instead of https://github.com/actions/setup-ruby as it supports more versions (including Ruby 3.0), so it should be fine! Can you try adding the version?

ball-hayden commented 3 years ago

@mrcasals Thanks for your review.

That's Ruby 3.0 running in CI, and a good job too - it caught a few other places 🙂. All updated now.

ball-hayden commented 3 years ago

@mrcasals would you be able to take another look at this, please?

thibaudgg commented 3 years ago

It would be great to see that patch merged and a new release of the gem. I'm running it on production, no issues so far. 👍🏻

mrcasals commented 3 years ago

Hi @ball-hayden I'm seeing some CI errors 😞 any idea what's going on?

ball-hayden commented 3 years ago

The error there looks to be an intermittent flake - I can reproduce on master by running e.g.

TESTOPTS="--seed=7220" ACTIVE_MODEL_VERSION=5.2 bundle exec rake test
# or
TESTOPTS="--seed=50616" ACTIVE_MODEL_VERSION=5.2 bundle exec rake test

I did have a quick look to see if I could trace it, but I didn't get that far. I'd guess there's state that's leaking between tests?

mrcasals commented 3 years ago

I'll try rerunning the test suite!

n-rodriguez commented 3 years ago

Hi there! any news?

ball-hayden commented 3 years ago

I'm afraid I haven't had a look into the test issues.

They are reproducible as described above - if you can find what's going on that would help move this PR forwards 🙂

ball-hayden commented 3 years ago

I'm hopeful https://github.com/codegram/date_validator/pull/86 will fix the flakes we're seeing in CI. If that is mergeable I will rebase this branch and, hopefully, we'll be good to go with this PR ✅.

mrcasals commented 3 years ago

Hi @ball-hayden! I merged #86, can you rebase this one? 😄

ball-hayden commented 3 years ago

🎉

ball-hayden commented 3 years ago

@mrcasals would you mind having another look now this has passed CI?

mrcasals commented 3 years ago

Thanks, @ball-hayden and sorry for the delay on this!

dwightwatson commented 3 years ago

Any chance of getting a new versioned release with this fix?

ball-hayden commented 3 years ago

Sorry to bump @mrcasals, but is there any chance of a release please?

ccdredzik commented 3 years ago

@mrcasals bump for interest

mrcasals commented 3 years ago

@dwightwatson @ball-hayden @ccdredzik new version just released! Check 0.11.0:

https://rubygems.org/gems/date_validator

Thanks for your patience!