Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
vmuriart merged 3 commits intopythonnet:masterfromvmuriart:method_object
Feb 22, 2017

Conversation

vmuriart
Copy link
Contributor

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

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

@den-run-ai
Copy link
Contributor

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

@vmuriart
Copy link
ContributorAuthor

@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
Copy link
Contributor

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

#265

@vmuriart
Copy link
ContributorAuthor

That sounds much broader than the scope of thispull_request.
I'm planning on refactoringmethodbinder to clean up alot of what is going in there.

@den-run-ai
Copy link
Contributor

@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
Copy link
ContributorAuthor

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

@vmuriart
Copy link
ContributorAuthor

@denfromufa Multiple arguments with objects works as expected.

@den-run-ai
Copy link
Contributor

den-run-ai commentedFeb 15, 2017
edited
Loading

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

@vmuriartvmuriartforce-pushed themethod_object branch 3 times, most recently from17499ae toc9ab4eaCompareFebruary 15, 2017 07:24
@vmuriart
Copy link
ContributorAuthor

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

@den-run-ai
Copy link
Contributor

den-run-ai commentedFeb 15, 2017 via email

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

@vmuriart
Copy link
ContributorAuthor

@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

@den-run-ai
Copy link
Contributor

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

@vmuriart
Copy link
ContributorAuthor

vmuriart commentedFeb 15, 2017
edited
Loading

@denfromufaint are also a subclass ofSystem.Object.

System.Object   System.ValueType      System.Int32

Sources:

@den-run-ai
Copy link
Contributor

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
Copy link
Contributor

den-run-ai commentedFeb 16, 2017
edited
Loading

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

@vmuriart
Copy link
ContributorAuthor

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

@@ -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)
Copy link
Contributor

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?

@den-run-ai
Copy link
Contributor

@vmuriart I left 2 comments inline with the code

@vmuriart
Copy link
ContributorAuthor

vmuriart commentedFeb 18, 2017 via email

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

@den-run-ai
Copy link
Contributor

den-run-ai commentedFeb 18, 2017 via email

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

@vmuriart
Copy link
ContributorAuthor

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

@den-run-ai
Copy link
Contributor

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
Copy link
ContributorAuthor

There was only a single comment.

image

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

@den-run-ai
Copy link
Contributor

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

so you don't see this?

image

@vmuriart
Copy link
ContributorAuthor

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

Copy link
Contributor

@den-run-aiden-run-ai left a 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")
Copy link
Contributor

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";        }

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Now its there.

@codecov
Copy link

codecovbot commentedFeb 20, 2017
edited
Loading

Codecov Report

Merging#377 intomaster willincrease coverage by0.07%.
The diff coverage is100%.

@@            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
FlagCoverage Δ
#Embedded_Tests32.57% <100%> (-0.02%)
#Python_Tests60.64% <100%> (+0.07%)
#Setup_Linux74% <100%> (ø)
#Setup_Windows70.5% <100%> (ø)
Impacted FilesCoverage Δ
src/runtime/methodbinder.cs90.61% <100%> (ø)
src/runtime/converter.cs77.89% <ø> (+1.05%)

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last updatee753b72...0ae9c38. Read thecomment docs.

@vmuriart
Copy link
ContributorAuthor

Added test and its passing as well.

return "Got object-int";
}

public static string TestOverloadedObjectTwo(object a, object b)
Copy link
Contributor

@den-run-aiden-run-aiFeb 21, 2017
edited
Loading

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

Copy link
ContributorAuthor

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.

@vmuriart
Copy link
ContributorAuthor

@denfromufa test added and passing as well.

@den-run-ai
Copy link
Contributor

@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
Copy link
ContributorAuthor

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

@vmuriartvmuriartforce-pushed themethod_object branch 3 times, most recently fromdb6c2ac to0ae9c38CompareFebruary 21, 2017 19:45
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@den-run-aiden-run-aiden-run-ai left review comments

@filmorfilmorfilmor approved these changes

@tonyrobertstonyrobertsAwaiting requested review from tonyroberts

Assignees

@vmuriartvmuriart

Labels
None yet
Projects
None yet
Milestone
2.3.0
Development

Successfully merging this pull request may close these issues.

Pythonnet 2.1 not identifying System.Object type
3 participants
@vmuriart@den-run-ai@filmor

[8]ページ先頭

©2009-2025 Movatter.jp