Implement isinstance - #122
Conversation
Codecov Report
@@ Coverage Diff @@
## master #122 +/- ##
==========================================
- Coverage 72.85% 72.74% -0.12%
==========================================
Files 60 60
Lines 11949 11970 +21
==========================================
+ Hits 8706 8707 +1
- Misses 2709 2729 +20
Partials 534 534
Continue to review full report at Codecov.
|
corona10
left a comment
corona10
left a comment
There was a problem hiding this comment.
Please rebase your branch first :)
ncw
left a comment
ncw
left a comment
There was a problem hiding this comment.
I think this basically looks sound :-)
I put some comments inline.
I didn't know isinstance recursively unpacked its args - who would have thought!
| var res py.Bool | ||
| var class = args[1].(py.Tuple) | ||
| for idx := range class { | ||
| var new_args []py.Object |
There was a problem hiding this comment.
This would be more efficently written
var new_args = []py.Object{args[0], class[idx]}
| if err != nil { | ||
| return false, err | ||
| } | ||
| res = res || temp |
There was a problem hiding this comment.
This should say
if res {
return res
}to short circuit the rest of the evaluations shouldn't it?
If you do that you can move the definition of res to here.
| } | ||
| res = res || temp | ||
| } | ||
| return res, nil |
There was a problem hiding this comment.
Then this can say
return false, nil| } | ||
| default: | ||
| { | ||
| if args[0].Type() != py.TypeType { |
There was a problem hiding this comment.
You are checking this for every entry in the args - it would be better checked in builtin_isinstance I think.
ncw
left a comment
ncw
left a comment
There was a problem hiding this comment.
I think this looks fine now except for the spare debug!
Can you fix that, squash into one commit and force push, then I'll merge - thank you :-)
|
I think this is looking good now - thank you :-) Apologies for the delay! I'll merge it now. |
4 participants
#121