Skip to content

Implement isinstance - #122

Merged
ncw merged 1 commit into
go-python:masterfrom
xarus01:master
Nov 5, 2019
Merged

Implement isinstance#122
ncw merged 1 commit into
go-python:masterfrom
xarus01:master

Conversation

@xarus01

Copy link
Copy Markdown
Contributor

@codecov-io

codecov-io commented Oct 20, 2019

Copy link
Copy Markdown

Codecov Report

Merging #122 into master will decrease coverage by 0.11%.
The diff coverage is 4.76%.

Impacted file tree graph

@@ 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
Impacted FilesCoverage Δ
builtin/builtin.go77.53% <4.76%> (-2.68%)⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c23b14...5558a8b. Read the comment docs.

@corona10corona10 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rebase your branch first :)

Comment threadbuiltin/builtin.go Outdated

@ncwncw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this basically looks sound :-)

I put some comments inline.

I didn't know isinstance recursively unpacked its args - who would have thought!

Comment threadbuiltin/builtin.go Outdated
Comment threadbuiltin/builtin.go Outdated
Comment threadbuiltin/builtin.go Outdated
Comment threadbuiltin/builtin.go Outdated
var res py.Bool
var class = args[1].(py.Tuple)
for idx := range class {
var new_args []py.Object

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be more efficently written

var new_args = []py.Object{args[0], class[idx]}

Comment threadbuiltin/builtin.go Outdated
if err != nil {
return false, err
}
res = res || temp

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should say

ifres {
returnres
}

to short circuit the rest of the evaluations shouldn't it?

If you do that you can move the definition of res to here.

Comment threadbuiltin/builtin.go Outdated
}
res = res || temp
}
return res, nil

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then this can say

returnfalse, nil

Comment threadbuiltin/builtin.go Outdated
}
default:
{
if args[0].Type() != py.TypeType {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are checking this for every entry in the args - it would be better checked in builtin_isinstance I think.

@ncwncw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :-)

Comment threadbuiltin/builtin.go Outdated

@corona10corona10 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @xarus01

I left some nit review.
cc @ncw

Comment threadvm/tests/builtin.py Outdated
Comment threadbuiltin/builtin.go
@ncw

ncw commented Nov 5, 2019

Copy link
Copy Markdown
Collaborator

I think this is looking good now - thank you :-)

Apologies for the delay!

I'll merge it now.

@ncw
ncw merged commit 051f189 into go-python:masterNov 5, 2019
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@xarus01@codecov-io@ncw@corona10