All CLA requirements met.

Just to note that we have been using this fix for some time and it does appear to work. It is a welcome change from needing to specify long list of args you don't care about.

Just FYI, there's a much better way to do method resolution (which I hope we will adopt at some point) but it requires lots of rework: C# Binder. I did a simple experiment here: losttech@605c576

We can still take this change in meanwhile.

Yes this wasn't supposed to change the world, simply just to get something that worked 😄

@jmlidbetter we have released 2.5 and master is now working on 3.x. Can you update this PR, so we could merge it?

@jmlidbetter we have released 2.5 and master is now working on 3.x. Can you update this PR, so we could merge it?

Sure. I assume just rebasing on top of master is sufficient or is there something else you'd want?

@jmlidbetter it should be. I'll re-review just in case.

@lostmsu Ok I have rebased, feel free to check and merge (as appropriate)

@jmlidbetter related tests seem to be failing.

Hmm that is strange - when I ran the tests on my machine they all passed.... I will have a look tomorrow.

Seems like the Linux tests have passed ok but are broken on Windows - perhaps there is some slight difference in behaviour on windows. I have limited ability test on windows but I'll see what I can do.

@lostmsu I have not been able to reproduce the test failures. Building on Windows I get the following output:

Are you able to repro this issue on windows?

I am not sure what causes the discrepancy, but judging by the code, it seems like the CI build messes up with named parameters. I will try to see if that is correct.

However, this also uncovers a bug: if Python passed a named parameter not defined in C#, the overload resolution behavior is incorrect. I suggest to handle that and add a test too.

void NoOverloads(int a = 0) {}

NoOverloads(c=1) # should fail, because c does not match a

I believe I had originally implemented that behaviour (not submitted onto this branch) but it broke some other tests. I may be misremembering though, I will check tomorrow.

@lostmsu here is the breaking test if you fail when a kwarg is supplied that doesn't appear in the method signature:

____________________________________________ test_namespace_and_init _____________________________________________

    def test_namespace_and_init():
        calls = []
        class TestX(System.Object):
            __namespace__ = "test_clr_subclass_with_init_args"
            def __init__(self, *args, **kwargs):
                calls.append((args, kwargs))
>       t = TestX(1,2,3,foo="bar")
E       TypeError: No constructor matches given arguments: (<class 'int'>, <class 'int'>, <class 'int'>)

src/tests/test_subclass.py:229: TypeError

I haven't done any further investigation into this. I guess there are two ways to proceed:
(1) Log this as an issue and proceed as is
(2) Dig in and fix the broken test

Its up to you, but personally I'm in favour of (1) because (a) having working kwargs (except in the case where you supply extra ones ;) ) is quite a big improvement and (b) python is fairly loose anyway with ducktyping etc etc, in a way all C# methods would have an implicit **kwargs in their signature - it might be possible to throw a warning when extraneous kwargs are given, I don't know.

Are we good to merge this change once the tests have passed?

This looks good to me, @lostmsu are you also fine with merging this?

Codecov Report

Merging #1136 (8d0fc2c) into master (7870a9f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1136   +/-   ##
=======================================
  Coverage   87.97%   87.97%           
=======================================
  Files           1        1           
  Lines         291      291           
=======================================
  Hits          256      256           
  Misses         35       35           

Flags with carried forward coverage won't be shown. Click here to find out more.

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 7870a9f...8d0fc2c. Read the comment docs.