- Notifications
You must be signed in to change notification settings - Fork752
Refactoring of MethodBinder.Bind#829
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
The build failure seems to be simple build timeout. Very unlikely caused by this change. |
I'll have a closer look at this at the weekend. No idea why travis is not getting triggered here, I've poked the one part that timed out though, so this one should go green. Thanks already :) |
Codecov Report
@@ Coverage Diff @@## master #829 +/- ##==========================================- Coverage 76.85% 76.11% -0.75%========================================== Files 63 63 Lines 5786 5798 +12 Branches 943 950 +7 ==========================================- Hits 4447 4413 -34- Misses 1025 1071 +46 Partials 314 314
Continue to review full report at Codecov.
|
codecovbot commentedMar 13, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## master #829 +/- ##=========================================+ Coverage 76.85% 76.9% +0.04%========================================= Files 63 63 Lines 5786 5798 +12 Branches 943 950 +7 =========================================+ Hits 4447 4459 +12 Misses 1025 1025 Partials 314 314
Continue to review full report at Codecov.
|
I'll just close and reopen this one to poke Travis... |
lostmsu commentedMar 14, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
CI still having internal problems :/ |
| for (var v = pynargs; v < clrnargs; v++) | ||
| //CLRObject co = (CLRObject)ManagedType.GetManagedObject(inst); | ||
| // InvalidCastException: Unable to cast object of type | ||
| // 'Python.Runtime.ClassObject' to type 'Python.Runtime.CLRObject' |
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.
This comment confuses me more than it helps ;). It's clear after reading the next one, but here not so much.
| { | ||
| // TODO: is this a bug? Should this happen even if the conversion fails? | ||
| // GetSlice() creates a new reference but GetItem() | ||
| // returns only a borrow reference. |
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.
This indeed looks like a bug to me, the code has to also run beforereturn null.
I think both comments are for things that you didn't change, just wanted to leave them around. |
Uh oh!
There was an error while loading.Please reload this page.
What does this implement/fix? Explain your changes.
This is a refactoring/cleaning up of MethodBinder.Bind method. No new features were introduced.
Does this close any currently open issues?
This is a preliminary work necessary for#823.
Any other comments?
The refactoring was mostly done using ReSharper with little manual interventions. The semantics should be preserved.
Checklist
Check all those that are applicable and complete.
AUTHORSCHANGELOG