-
Notifications
You must be signed in to change notification settings - Fork 232
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
Handle when the passed value is a Numeric value #229
base: master
Are you sure you want to change the base?
Handle when the passed value is a Numeric value #229
Conversation
@@ -10,7 +10,7 @@ def initialize(type:, format: nil, ignore_usec: false, time_zone_aware: false) | |||
end | |||
|
|||
def type_cast_value(value) | |||
return nil if value.nil? || !value.respond_to?(:to_time) | |||
return nil if value.nil? || !value.respond_to?(:to_time) || value.is_a?(Numeric) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a failing spec?
At the moment I do not understand how it is possible that a Numeric
value passes !value.respond_to?(:to_time)
pry(main)> ValidatesTimeliness::VERSION
=> "7.0.0.beta1"
pry(main)> String.method_defined?(:to_time)
=> true
pry(main)> Numeric.method_defined?(:to_time)
=> false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# String
pry(main)> "444".respond_to?(:to_time)
=> true
pry(main)> "444".respond_to?(:to_date)
=> true
# Integer
pry(main)> 444.respond_to?(:to_time)
=> true
pry(main)> 444.respond_to?(:to_date)
=> false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which would mean this guard would still fail, because the value is actually a string.
>> "444.0".is_a?(Numeric)
false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But as @tagliala said. we'll need a failing spec for this. It is not clear how this occurs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only thing that I can think of is that Numeric
has been enhanced with to_time
by another library, but that same gem does not define to_date
. However, even in this use case that would fail the next line when methods like acts_like?(:time)
are invoked. This is why a reduced failing test case would help
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example:
# app/models/item.rb
validates_date :expire_date
# console:
> item = Item.new(expire_date: 444)
> item.valid?
NoMethodError: undefined method `to_date' for 444:Integer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks for the clarification.
The
Numeric
values are responds to"to_time"
but not"to_date"
.
This is the part that does not sound.
Is there any third party library defining to_time
on Numeric elements? Can you check where to_time
is defined?
I've setup a pretty basic application at https://github.com/diowa/ruby3-rails7-bootstrap-heroku/tree/validates-timeliness
> 444.respond_to?(:to_time)
=> false
[2] pry(main)> Item.new(expire_date: 444).valid?
=> false
However... @adzap probably there could be a better detection if the timeliness
helpers are available on the given value. This can also improve third-party compatibility
Run `Item.new(expire_date: 444).valid?` in console
When the passed value is a Numeric value, then we get an exception 👇
=>
NoMethodError: undefined method "to_date" for 444:Integer
So, this PR is to handle that case!
@adzap I hope you like this tiny PR and merge it.
Regards 🎉