- Notifications
You must be signed in to change notification settings - Fork752
Method overload object type#377
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
@vmuriart what happens with multiple arguments, when overloaded methods have |
@denfromufa good question. No idea.
These 4 functions look enough to cover the possibilities or do I need more? |
we need one super-loaded method which covers all these cases from this meta-issue :) |
That sounds much broader than the scope of this |
@vmuriart I understand, your example looks good. My suggestion was not serious anyway. But we may hit issues when dealing with sub-classes and ambiguous cases in the overload. IMO, pythonnet should be very strict by default in method resolution. See examples how C# compiler had to break method overloading in .NET 4.0: |
@denfromufa No worries. My goal is to restore similar behavior to what |
@denfromufa Multiple arguments with objects works as expected. |
den-run-ai commentedFeb 15, 2017 • 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.
@vmuriart no failure cases in overloads to make sure that they fail. No strings in overloaded method arguments when creating the methods in c#, but then you use strings as arguments when invoking from python. |
17499ae
toc9ab4ea
CompareI think I understood your request correctly. I added a test to check for failure/exception. |
Can you add more overloaded methods such as:```public static string TestOverloadedObjectTwo(string a, string b){return"Got string-string";}```Which one would the method resolution pick object or string? Does the orderof defined methods matter?Wrong result would be much worse than flagging ambiguity or failure inmethod resolution. …On Wed, Feb 15, 2017, 1:26 AM Victor Uriarte ***@***.***> wrote: I think I understood your request correctly. I added a test to check for failure/exception. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#377 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHgZ5ZWYSupa6rOYtu-trQflb6-85xhrks5rcqiogaJpZM4MBRiU> . |
@denfromufa I think you are asking forthese tests I added them on aseparate commit. Earlier when I was working on this pull request I used those tests to make sure I wasn't reverting the behavior you are describing. On earlier implementations, I reverted the behavior so those tests kept me in check. The string-string check is analogous to theint-int check vsobj-obj check |
@vmuriart string check is not analogous, you are assuming that strings are object in your tests. In fact string is subclass of object. |
vmuriart commentedFeb 15, 2017 • 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.
@denfromufa
Sources: |
yes, int is also object subclass. However you use int in C# overloaded methods, while not using strings. Let me give some examples. |
den-run-ai commentedFeb 16, 2017 • 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.
@vmuriart here is what I suggested to add. I cannot test locally, due to#385 publicstaticstringTestOverloadedObject(objecto){return"Got object";}publicstaticstringTestOverloadedObjectTwo(inta,intb){return"Got int-int";}publicstaticstringTestOverloadedObjectTwo(stringa,stringb){return"Got string-string";}publicstaticstringTestOverloadedObjectTwo(stringa,intb){return"Got string-int";}publicstaticstringTestOverloadedObjectTwo(stringa,objectb){return"Got string-object";}publicstaticstringTestOverloadedObjectTwo(inta,objectb){return"Got int-object";}publicstaticstringTestOverloadedObjectTwo(objecta,intb){return"Got object-int";}publicstaticstringTestOverloadedObjectTwo(objecta,objectb){return"Got object-object";}publicstaticstringTestOverloadedObjectThree(objecta,intb){return"Got object-int";}publicstaticstringTestOverloadedObjectThree(inta,objectb){return"Got int-object";} deftest_object_in_multiparam():"""Test method with object multiparams behaves"""res=MethodTest.TestOverloadedObjectTwo(5,5)assertres=="Got int-int"res=MethodTest.TestOverloadedObjectTwo(5,"foo")assertres=="Got int-object"res=MethodTest.TestOverloadedObjectTwo("foo",5)assertres=="Got object-int"res=MethodTest.TestOverloadedObjectTwo("foo","bar")assertres=="Got object-object"res=MethodTest.TestOverloadedObjectTwo("foo",System.Object)assertres=="Got string-object"res=MethodTest.TestOverloadedObjectTwo("foo","bar")assertres=="Got string-string"res=MethodTest.TestOverloadedObjectTwo("foo",5)assertres=="Got string-int" |
@denfromufa Added your tests, but had to modify remove a couple of the conditions I had added earlier since the function signatures had changed. |
src/runtime/methodbinder.cs Outdated
@@ -375,7 +375,7 @@ internal Binding Bind(IntPtr inst, IntPtr args, IntPtr kw, MethodBase info, Meth | |||
if (clrtype != null) | |||
{ | |||
var typematch = false; | |||
if (pi[n].ParameterType != clrtype) | |||
if (pi[n].ParameterType !=typeof(object) && pi[n].ParameterType !=clrtype) |
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.
can you add brackets around each logical?
@vmuriart I left 2 comments inline with the code |
I'll review on Tuesday. Though the parenthesis seem superfluous since itisn't consistent throughout. …On Fri, Feb 17, 2017 at 18:56 denfromufa ***@***.***> wrote:@vmuriart <https://github.com/vmuriart> I left 2 comments inline with the code — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#377 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMr87EoUuLcuTE5k0mXi3kl9MNwyHCvPks5rdl3GgaJpZM4MBRiU> . |
yes, brackets is very minor. But I feel the same way as this person:http://stackoverflow.com/a/23816736/2230844On Fri, Feb 17, 2017 at 9:28 PM, Victor Uriarte <notifications@github.com>wrote: … I'll review on Tuesday. Though the parenthesis seem superfluous since it isn't consistent throughout. On Fri, Feb 17, 2017 at 18:56 denfromufa ***@***.***> wrote: >@vmuriart <https://github.com/vmuriart> I left 2 comments inline with the > code > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#377 (comment) >, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ AMr87EoUuLcuTE5k0mXi3kl9MNwyHCvPks5rdl3GgaJpZM4MBRiU> > . > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#377 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHgZ5QThkeZd3Re4qve47oNeO1lnknICks5rdmVVgaJpZM4MBRiU> . |
@denfromufa Added the parenthesis. Good to squash-merge? |
you did not address the second comment.@vmuriart will this break if you add:
|
It's not there. It even says on your screen shot that its |
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.
testing
res = MethodTest.TestOverloadedObjectTwo(5, 5) | ||
assert res == "Got int-int" | ||
res = MethodTest.TestOverloadedObjectTwo(5, "foo") |
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.
will this break if you add:
public static string TestOverloadedObjectTwo(int a, string b) { return "Got int-string"; }
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.
Now its there.
codecovbot commentedFeb 20, 2017 • 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 #377 +/- ##==========================================+ Coverage 63.27% 63.35% +0.07%========================================== Files 61 61 Lines 5226 5226 Branches 860 860 ==========================================+ Hits 3307 3311 +4+ Misses 1697 1695 -2+ Partials 222 220 -2
Continue to review full report at Codecov.
|
Added test and its passing as well. |
return "Got object-int"; | ||
} | ||
public static string TestOverloadedObjectTwo(object a, object b) |
den-run-aiFeb 21, 2017 • 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.
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.
you don't test this method in python
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.
because you changed the test for it above. I re-added a test for it.
@denfromufa test added and passing as well. |
@vmuriart I'm still surprised that this does not break ;) i approve on my side. in the future we should definitely improve testing all these combinations of arguments by using hypothesis or parametrizing pytest. Let's release v2.3.0.dev1 version after this merge. @filmor@tonyroberts please review! |
@denfromufa I didn't want to introduce |
db6c2ac
to0ae9c38
Compare
What does this implement/fix? Explain your changes.
Fixes Method object overloading without breaking changes introduced by#131 and#151.
I added the tests/examples from#131 to thetests to ensure that the behavior didn't get reverted.
Does this close any currently open issues?
Closes#203
Any other comments?
This patch fixes the issue from#203, but
MethodBinder.Bind
needs rewriting since its basically been patched over and over again to fix edge cases. I didn't want to do it as part of thispull_request
since it would affect more than just fixing#203.Checklist
Check all those that are applicable and complete.
AUTHORS