-
-
Notifications
You must be signed in to change notification settings - Fork 396
Match error messages for TypeError in File specs #1345
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
base: master
Are you sure you want to change the base?
Conversation
|
Kind of a sequel to truffleruby/truffleruby#4140, it turned out the error handling in Natalie is all over the place too. |
|
Okay, so it looks like I would consider this a bug in MRI. |
|
Yeah those inconsistencies are pretty annoying. |
Though from the diff A quick search gives https://github.com/search?q=repo%3Aruby%2Fruby+%2Fno+implicit+conversion+%2F+path%3A*.c&type=code |
30a44a6 to
96434d2
Compare
There are two different code paths: true/false/nil uses the default inspect string in the error message, other objects use the class name. The conversion into Integer is even more inconsistent, where it's sometimes "from X to integer" and sometimes "of X into Integer".
96434d2 to
7e39924
Compare
There is also the annoying capitalization issue of "integer" vs "Integer", etc. Considering that generic message uses class's name, it would be nice if the custom messages were also consistent in that regard. |
|
Upstream case: https://bugs.ruby-lang.org/issues/21864 |
| -> { File.truncate(@name, nil) }.should raise_error(TypeError, /no implicit conversion from nil( to integer)?/) | ||
| -> { File.truncate(@name, "") }.should raise_error(TypeError, /no implicit conversion (of String into Integer|from string)/) |
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.
Could you allow no implicit conversion of X into Integer here?
I think other Rubies should use that for consistency and should be able to pass this spec with that.
Also will make it easier if https://bugs.ruby-lang.org/issues/21864 gets fixed.
|
|
||
| it "raises a TypeError if not passed an Integer type for the for the argument" do | ||
| -> { @file.truncate(nil) }.should raise_error(TypeError) | ||
| -> { @file.truncate(nil) }.should raise_error(TypeError, /no implicit conversion from nil( to 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.
Same, could you allow no implicit conversion of nil into Integer here?
|
Given ruby/ruby#16078 (comment), let's use |
|
@herwinw Pushed to ruby/mspec@4ea4bf7, could you use it here? |
There are two different code paths: true/false/nil uses the default inspect string in the error message, other objects use the class name.
The conversion into Integer is even more inconsistent, where it's sometimes "from X to integer" and sometimes "of X into Integer".