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.

Sign up for GitHub

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

Jump to bottom

Handle when the passed value is a Numeric value #229

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

Handle when the passed value is a Numeric value #229

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/validates_timeliness/converter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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

@eng-Abdurhman eng-Abdurhman Feb 22, 2023 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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


value = value.in_time_zone if value.acts_like?(:time) && time_zone_aware?
value = case type
Expand Down