Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle when the passed value is a Numeric value #229

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

eng-Abdurhman
Copy link

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 馃帀

@@ -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)
Copy link
Contributor

@tagliala tagliala Feb 16, 2023

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tagliala 馃憞

# 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

Copy link
Owner

@adzap adzap Feb 23, 2023

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

Copy link
Owner

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.

Copy link
Contributor

@tagliala tagliala Feb 23, 2023

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adzap @tagliala
The condition in line #13 just check whether the passed value responds to "to_time" method or not.
whereas the issue is in line #20 while calling "to_date" method not "to_time"!

The Numeric values are responds to "to_time" but not "to_date".

Copy link
Author

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

Copy link
Contributor

@tagliala tagliala Feb 28, 2023

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

tagliala added a commit to diowa/ruby3-rails7-bootstrap-heroku that referenced this pull request Feb 28, 2023
Run `Item.new(expire_date: 444).valid?` in console
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants