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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Error message's when passing nil args when initializing a Date #2272

Closed

Conversation

dannyd4315
Copy link

@dannyd4315 dannyd4315 commented Jul 2, 2019

This PR fixes an issue with Date and passing nil arguments when initializing a Date. The issue is explained here - https://bugs.ruby-lang.org/issues/15298

Current behaviour

Date.new(1999, 1, 32)   # => ArgumentError: invalid date

If you provide nil as any of the three arguments, you do not get this error. You get three unrelated errors (two NoMethodErrors and one TypeError), none of which are specific to date:

Date.new(1999, 1, nil)  # => NoMethodError: undefined method `div' for nil:NilClass
Date.new(1999, nil, 32) # => TypeError: no implicit conversion from nil to integer
Date.new(nil, 1, 32)    # => NoMethodError: undefined method `<' for nil:NilClass

New behaviour

Date.new(1999, 1, 32)   # => ArgumentError: invalid date
Date.new(1999, 1, nil)  # => ArgumentError: invalid date
Date.new(1999, nil, 32) # => ArgumentError: invalid date
Date.new(nil, 1, 32)    # => ArgumentError: invalid date

@jeremyevans
Copy link
Contributor

It seems odd to me to only attempt to handle nil values specially. Shouldn't Date.new(Object.new, 1, 1) raise the same exception as Date.new(nil, 1, 1)? Additionally, if we are going to check that the types do not match, TypeError seems to be a better error to raise than ArgumentError. Maybe we should try converting all values to integer, so we get:

Date.new(1999, 1, nil)  # => TypeError: no implicit conversion from nil to integer
Date.new(1999, nil, 1) # => TypeError: no implicit conversion from nil to integer
Date.new(nil, 1, 1)    # => TypeError: no implicit conversion from nil to integer

That seems to be more consistent with the core classes:

Array.new(nil)
"" * nil
[1,2,3][1, nil]

nobu pushed a commit to nobu/ruby that referenced this pull request Jul 4, 2019
@shyouhei
Copy link
Member

It seems ruby/date#9 is related.

@dannyd4315 dannyd4315 closed this Jan 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants