-
Notifications
You must be signed in to change notification settings - Fork 605
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
Guards for BasicObject when coercing types #3077
Conversation
This way it matches the message emitted by MRI.
Rational's second argument requires an extra guard in case BasicObject is used. While this is a rare case MRI raises a specific error in this case. The actual error message ("not an integer") is rather useless so we're using a (hopefully) slightly more useful one.
@@ -47,6 +47,23 @@ def self.coerce_to_type_error(original, converted, method, klass) | |||
end | |||
|
|||
## | |||
# BasicObject responds to pretty much no, if any methods at all. This |
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.
it doesn't respond to that many methods, but to say "if any methods at all" is probably an exaggeration. i count 12 instance methods and a single class method(::new). the documentation on the left hand side of this webpage http://www.ruby-doc.org/core-2.1.2/BasicObject.html has a list of methods. indeed its very few but to say any methods at all is probably a little misleading.
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.
@rpag Hm good point, I'll clarify the message a bit.
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.
Ok, cleared up the docs a tad in d765183.
@brixen At Baruco we briefly talked about this PR, but I can't exactly remember whether or not we reached any particular form of consensus on this pull request. Personally I have mixed feelings about this. If we have to add guards everywhere just for |
I'm closing this one. I discussed this particular topic at Baruco with @brixen and have given it some thought since. The conclusion is that there's no sane way (currently) to deal with this. In this particular PR we added guards for BasicObject, but there could be a theoretical infinite amount of cases we'd have to handle (in various different places). The only sane way is to add a hinting system that says what a method can deal with, opposed to what it can't deal with (so basically some form of static typing, but based on behaviour instead of literal types). |
This PR adds a set of guards for BasicObject when using method such as
Kernel.Float
andKernel.Hash
. MRI has special handling for BasicObject. Rubinius didn't and would raise NoMethodError erorrs instead. This PR also includes an extra guard for the second argument ofKernel.Rational
.I'm not merging this straight in as I'd like to have some feedback on this matter first. The guards have to be added there in the end but I'm curious if there's perhaps a better way of doing things (or just a more preferred way).