Method overload object type #377

vmuriart · 2017-02-15T03:47:37Z

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 the tests 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 this pull_request since it would affect more than just fixing #203.

Checklist

Check all those that are applicable and complete.

Make sure to include one or more tests for your change
Add yourself to AUTHORS

den-run-ai · 2017-02-15T05:17:23Z

@vmuriart what happens with multiple arguments, when overloaded methods have object argument?

vmuriart · 2017-02-15T05:21:54Z

@denfromufa good question. No idea.

foo(int a, object b)
foo(object a, int b)
foo(object a, object b)
foo(int a, int b)

These 4 functions look enough to cover the possibilities or do I need more?

den-run-ai · 2017-02-15T05:25:35Z

we need one super-loaded method which covers all these cases from this meta-issue :)

#265

vmuriart · 2017-02-15T05:36:17Z

That sounds much broader than the scope of this pull_request.
I'm planning on refactoring methodbinder to clean up alot of what is going in there.

den-run-ai · 2017-02-15T05:44:16Z

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

http://csharpindepth.com/Articles/General/Overloading.aspx

vmuriart · 2017-02-15T05:53:30Z

@denfromufa No worries. My goal is to restore similar behavior to what pythonnet==2.0.0 had irt method objects. I had to freeze the version of pythonnet I was using to v2.0 because of it.
I'm ok w switching to a stricter method resolution, as long as it is part of v3.0 😄

vmuriart · 2017-02-15T06:34:56Z

@denfromufa Multiple arguments with objects works as expected.

den-run-ai · 2017-02-15T06:54:46Z

@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.

vmuriart · 2017-02-15T07:26:31Z

I think I understood your request correctly. I added a test to check for failure/exception.

den-run-ai · 2017-02-15T07:52:05Z

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 order of defined methods matter? Wrong result would be much worse than flagging ambiguity or failure in method 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> .

vmuriart · 2017-02-15T08:03:47Z

@denfromufa I think you are asking for these tests I added them on a separate 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 the int-int check vs obj-obj check

den-run-ai · 2017-02-15T15:39:20Z

@vmuriart string check is not analogous, you are assuming that strings are object in your tests. In fact string is subclass of object.

vmuriart · 2017-02-15T15:45:59Z

@denfromufa int are also a subclass of System.Object.

System.Object
   System.ValueType
      System.Int32

Sources:

den-run-ai · 2017-02-15T16:01:41Z

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 · 2017-02-16T08:08:27Z

@vmuriart here is what I suggested to add. I cannot test locally, due to #385

public static string TestOverloadedObject(object o)
        {
            return "Got object";
        }

        public static string TestOverloadedObjectTwo(int a, int b)
        {
            return "Got int-int";
        }

        public static string TestOverloadedObjectTwo(string a, string b)
        {
            return "Got string-string";
        }

        public static string TestOverloadedObjectTwo(string a, int b)
        {
            return "Got string-int";
        }

        public static string TestOverloadedObjectTwo(string a, object b)
        {
            return "Got string-object";
        }

        public static string TestOverloadedObjectTwo(int a, object b)
        {
            return "Got int-object";
        }

        public static string TestOverloadedObjectTwo(object a, int b)
        {
            return "Got object-int";
        }

        public static string TestOverloadedObjectTwo(object a, object b)
        {
            return "Got object-object";
        }

        public static string TestOverloadedObjectThree(object a, int b)
        {
            return "Got object-int";
        }

        public static string TestOverloadedObjectThree(int a, object b)
        {
            return "Got int-object";
        }

def test_object_in_multiparam():
    """Test method with object multiparams behaves"""

    res = MethodTest.TestOverloadedObjectTwo(5, 5)
    assert res == "Got int-int"

    res = MethodTest.TestOverloadedObjectTwo(5, "foo")
    assert res == "Got int-object"

    res = MethodTest.TestOverloadedObjectTwo("foo", 5)
    assert res == "Got object-int"

    res = MethodTest.TestOverloadedObjectTwo("foo", "bar")
    assert res == "Got object-object"

    res = MethodTest.TestOverloadedObjectTwo("foo", System.Object)
    assert res == "Got string-object"
   
    res = MethodTest.TestOverloadedObjectTwo("foo", "bar")
    assert res == "Got string-string"

    res = MethodTest.TestOverloadedObjectTwo("foo", 5)
    assert res == "Got string-int"

vmuriart · 2017-02-17T03:55:20Z

@denfromufa Added your tests, but had to modify remove a couple of the conditions I had added earlier since the function signatures had changed.

den-run-ai · 2017-02-18T02:52:14Z

src/runtime/methodbinder.cs

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


can you add brackets around each logical?

den-run-ai · 2017-02-18T02:56:05Z

@vmuriart I left 2 comments inline with the code

vmuriart · 2017-02-18T03:28:20Z

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> .

den-run-ai · 2017-02-18T03:36:25Z

yes, brackets is very minor. But I feel the same way as this person: http://stackoverflow.com/a/23816736/2230844 On Fri, Feb 17, 2017 at 9:28 PM, Victor Uriarte <[email protected]> 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> .

vmuriart · 2017-02-20T23:18:45Z

@denfromufa Added the parenthesis. Good to squash-merge?

den-run-ai · 2017-02-20T23:27:41Z

you did not address the second comment. @vmuriart will this break if you add:

public static string TestOverloadedObjectTwo(int a, string b)
        {
            return "Got int-string";
        }

vmuriart · 2017-02-20T23:31:29Z

There was only a single comment.

And the answer is no. It simply changes the result of which method is used as it will pick the closest match it can find before it picked object.

den-run-ai · 2017-02-20T23:37:02Z

so can you add it? because testing closest match is important.

so you don't see this?

vmuriart · 2017-02-20T23:38:26Z

It's not there. It even says on your screen shot that its pending.

den-run-ai

testing

den-run-ai · 2017-02-18T02:55:04Z

src/tests/test_method.py

+    res = MethodTest.TestOverloadedObjectTwo(5, 5)
+    assert res == "Got int-int"
+
+    res = MethodTest.TestOverloadedObjectTwo(5, "foo")


will this break if you add:

public static string TestOverloadedObjectTwo(int a, string b) { return "Got int-string"; }

Now its there.

codecov · 2017-02-20T23:43:21Z

Codecov Report

Merging #377 into master will increase coverage by 0.07%.
The diff coverage is 100%.

@@            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

Flag	Coverage Δ
#Embedded_Tests	`32.57% <100%> (-0.02%)`	❌
#Python_Tests	`60.64% <100%> (+0.07%)`	✅
#Setup_Linux	`74% <100%> (ø)`	✅
#Setup_Windows	`70.5% <100%> (ø)`	✅

Impacted Files	Coverage Δ
src/runtime/methodbinder.cs	`90.61% <100%> (ø)`	✅
src/runtime/converter.cs	`77.89% <ø> (+1.05%)`	✅

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 e753b72...0ae9c38. Read the comment docs.

vmuriart · 2017-02-20T23:44:30Z

Added test and its passing as well.

den-run-ai · 2017-02-21T00:23:28Z

src/testing/methodtest.cs

+            return "Got object-int";
+        }
+
+        public static string TestOverloadedObjectTwo(object a, object b)


you don't test this method in python

because you changed the test for it above. I re-added a test for it.

vmuriart · 2017-02-21T00:35:20Z

@denfromufa test added and passing as well.

den-run-ai · 2017-02-21T03:06:56Z

@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!

vmuriart · 2017-02-21T03:12:51Z

@denfromufa I didn't want to introduce parametrizing tests yet. I figured I'd slowly introduce py.test features instead of all at once.

vmuriart self-assigned this Feb 15, 2017

vmuriart requested review from filmor, tonyroberts and den-run-ai February 15, 2017 03:47

vmuriart force-pushed the method_object branch 3 times, most recently from 17499ae to c9ab4ea Compare February 15, 2017 07:24

vmuriart force-pushed the method_object branch from c9ab4ea to e4f90ab Compare February 16, 2017 04:05

vmuriart force-pushed the method_object branch from e4f90ab to 75fc3ec Compare February 17, 2017 03:50

den-run-ai reviewed Feb 18, 2017

View reviewed changes

vmuriart force-pushed the method_object branch from 75fc3ec to 6f97e54 Compare February 20, 2017 23:17

den-run-ai reviewed Feb 20, 2017

View reviewed changes

den-run-ai reviewed Feb 21, 2017

View reviewed changes

vmuriart force-pushed the method_object branch 3 times, most recently from db6c2ac to 0ae9c38 Compare February 21, 2017 19:45

vmuriart added 3 commits February 22, 2017 00:06

Add object overload test

b7a3c7e

Add object type to methodbind

2cabc9f

Add more object overload method tests

4071aa3

vmuriart force-pushed the method_object branch from 0ae9c38 to 4071aa3 Compare February 22, 2017 07:07

filmor approved these changes Feb 22, 2017

View reviewed changes

vmuriart merged commit 8f258ac into pythonnet:master Feb 22, 2017

vmuriart deleted the method_object branch February 22, 2017 12:27

vmuriart added this to the 2.3.0 milestone Feb 22, 2017

sg08013 mentioned this pull request Feb 24, 2017

Compatibility issue between argparse and pythonnet packages #404

Closed

yagweb mentioned this pull request Mar 1, 2017

Exception when passing a custom python object as the parameter #412

Open

Method overload object type #377

Method overload object type #377

Conversation

vmuriart commented Feb 15, 2017

What does this implement/fix? Explain your changes.

Does this close any currently open issues?

Any other comments?

Checklist

den-run-ai commented Feb 15, 2017

vmuriart commented Feb 15, 2017

den-run-ai commented Feb 15, 2017

vmuriart commented Feb 15, 2017

den-run-ai commented Feb 15, 2017

vmuriart commented Feb 15, 2017

vmuriart commented Feb 15, 2017

den-run-ai commented Feb 15, 2017 • edited Loading

vmuriart commented Feb 15, 2017

den-run-ai commented Feb 15, 2017 via email

vmuriart commented Feb 15, 2017

den-run-ai commented Feb 15, 2017

vmuriart commented Feb 15, 2017 • edited Loading

den-run-ai commented Feb 15, 2017

den-run-ai commented Feb 16, 2017 • edited Loading

vmuriart commented Feb 17, 2017

den-run-ai Feb 18, 2017

Choose a reason for hiding this comment

den-run-ai commented Feb 18, 2017

vmuriart commented Feb 18, 2017 via email

den-run-ai commented Feb 18, 2017 via email

vmuriart commented Feb 20, 2017

den-run-ai commented Feb 20, 2017

vmuriart commented Feb 20, 2017

den-run-ai commented Feb 20, 2017

vmuriart commented Feb 20, 2017

den-run-ai left a comment

Choose a reason for hiding this comment

den-run-ai Feb 18, 2017

Choose a reason for hiding this comment

vmuriart Feb 20, 2017

Choose a reason for hiding this comment

codecov bot commented Feb 20, 2017 • edited Loading

Codecov Report

vmuriart commented Feb 20, 2017

den-run-ai Feb 21, 2017 • edited Loading

Choose a reason for hiding this comment

vmuriart Feb 21, 2017

Choose a reason for hiding this comment

vmuriart commented Feb 21, 2017

den-run-ai commented Feb 21, 2017

vmuriart commented Feb 21, 2017

den-run-ai commented Feb 15, 2017 •

edited

Loading

vmuriart commented Feb 15, 2017 •

edited

Loading

den-run-ai commented Feb 16, 2017 •

edited

Loading

codecov bot commented Feb 20, 2017 •

edited

Loading

den-run-ai Feb 21, 2017 •

edited

Loading