-
Notifications
You must be signed in to change notification settings - Fork 281
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
Fixes for several tests in test_bool #261
Conversation
Ast.IfThen(Ast.LessThan(res, Ast.Constant(0)), | ||
Ast.Throw( | ||
Ast.Call( | ||
typeof(PythonOps).GetMethod("ValueError"), |
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.
I wonder if we should really be throwing errors like that in the IL.
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.
We have to the way this is implemented. It doesn't use the normal len call, so it has to be thrown in here.
@@ -839,7 +845,7 @@ public static partial class PythonOps { | |||
} | |||
|
|||
if (res < 0) { | |||
throw PythonOps.ValueError("__len__ should return >= 0, got {0}", res); | |||
throw PythonOps.ValueError("__len__ should return >= 0"); |
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.
Missing the parentheses __len__()
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.
Good catch
@@ -470,6 +470,9 @@ public static partial class PythonOps { | |||
} | |||
|
|||
public static object Is(object x, object y) { | |||
if(x is bool && y is bool) { |
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.
What about reusing IronLanguages/main#1332 instead of having something different? Unless this fixes something else...
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.
Yeah, that's a way better idea. I'll revert what I have for that and merge.
No description provided.