Another shot at #495 #503

rickardraysearch · 2017-06-21T08:58:48Z

What does this implement/fix? Explain your changes.

Remove the extraneous call to __init__ from ClassDerived.InvokeCtor.

Does this close any currently open issues?

Any other comments?

This is a different shot at fixing #495 . @filmor has one at #496 but it fails a couple of tests. This one might have some bugs wrt XDecref in MetaType.tp_call still.

Checklist

Check all those that are applicable and complete.

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

mention-bot · 2017-06-21T08:58:50Z

@rickardraysearch, thanks! @vmuriart, @tonyroberts, @tiran, @ArvidJB and @brianlloyd, please review this.

codecov · 2017-06-21T09:35:39Z

Codecov Report

Merging #503 into master will increase coverage by 0.79%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #503      +/-   ##
==========================================
+ Coverage   76.41%   77.21%   +0.79%     
==========================================
  Files          64       64              
  Lines        5572     5534      -38     
  Branches      895      887       -8     
==========================================
+ Hits         4258     4273      +15     
+ Misses       1020      975      -45     
+ Partials      294      286       -8

Flag	Coverage Δ
#setup_linux	`75.71% <ø> (ø)`	⬆️
#setup_windows	`76.56% <100%> (+0.79%)`	⬆️

Impacted Files	Coverage Δ
src/runtime/runtime.cs	`91.6% <ø> (+0.76%)`	⬆️
src/runtime/classderived.cs	`88.34% <ø> (+7.91%)`	⬆️
src/runtime/metatype.cs	`71.55% <100%> (+0.82%)`	⬆️
src/runtime/classbase.cs	`80.95% <0%> (+2.85%)`	⬆️
src/runtime/delegateobject.cs	`65.9% <0%> (+11.36%)`	⬆️

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 c8ae358...db5f43e. Read the comment docs.

den-run-ai · 2017-06-21T10:32:44Z

src/tests/test_subclass.py

+    assert calls[0][0] == (1,2,3)
+    assert calls[0][1] == {"foo":"bar"}
+
+def test_clr_subclass_without_init_args():


please rename test_clr_subclass_init_without_args

filmor · 2017-06-21T10:35:02Z

src/runtime/metatype.cs

-            IntPtr type = Runtime.PyObject_TYPE(obj);
-            IntPtr init = Runtime._PyType_Lookup(type, py__init__);
-            Runtime.XDecref(py__init__);
+            var init = Runtime.PyObject_GetAttrString(obj, "__init__");


You are leaking this reference.

Oh. Should be fixed now.

filmor · 2017-06-21T10:37:55Z

Could you explain how and why this works?

/edit: Never mind, got it :)
@tonyroberts Do you know why the explicit (additional) call to __init__ was put into InvokeCtor in the first place?

rickardraysearch · 2017-06-21T10:47:56Z

I believe that what's happening is that MetaType.tp_call is calling __init__ on the class, just as PyType_Type does in type_call. It has access to all of the passed arguments, including those not available in the CLR constructor.

Since the class' __init__ is called from there, there is no need to call it from ClassDerived.InvokeCtor.

When constructing python objects from e.g. C#, the __init__ method would need to be called separately, since the CLR constructor only creates the object. If that should change, I believe we should have a look at actually calling __new__ for python types with __namespace__ and make those arguments match whatever is generated for the CLR constructors.

rickardraysearch · 2017-06-22T06:50:47Z

From the commit history, it looks as if the call was added to invoke __init__ when instantiating python classes from managed code. The tests that @tonyroberts added then still work though, so hopefully this PR isn't breaking any existing major use cases by removing the call to __init__ when constructing python objects from managed code.

By the way, there are two tests in test_subclass.py that are currently skipped with the comment FIXME: test randomly pass/fails. I have run them a couple of hundred times, both on macos and windows, with python 2.7 and 3.6, and not seen a single fail. Can I try reenabling them? They are both relevant to this PR.

/edit: I reenabled them, and they worked on both Travis and Appveyor, at least this time.

filmor · 2017-06-26T10:30:58Z

Thank you very much for your contribution. We'll have to see, whether this breaks compatibility for anyone, we probably should put a note into the changelog, documenting this change of behaviour.

rickardraysearch · 2017-06-26T10:33:42Z

Yes, it definitely makes sense to put a good explanation in the changelog. Also, could we have a discussion on how to bring back that functionality and exposing additional arguments to the managed constructors? Maybe a new issue is the best place for that.

Many thanks for merging this, by the way! It really makes a big difference for my chances of getting us to use pythonnet here.

filmor · 2017-06-26T10:36:03Z

Feel free to open one :)

* Add tests of subclassing with __namespace__ * Remove __init__ call from ClassDerived.InvokeCtor * Trying out __init__ * Cleanup * Add tests constructing python type from CLR and calling __init__ * Revert borked changelog update * Don't leak init reference * Rename tests * Remove unused internal Runtime.GetBoundArgTuple * Reenable skipped tests in test_subclass.py

Rickard Holmberg added 3 commits June 20, 2017 15:49

Add tests of subclassing with __namespace__

5f9a108

Remove __init__ call from ClassDerived.InvokeCtor

bc7b74b

Trying out __init__

7c9e881

Rickard Holmberg added 3 commits June 21, 2017 12:24

Cleanup

660b784

Add tests constructing python type from CLR and calling __init__

6e78a42

Revert borked changelog update

5308164

den-run-ai reviewed Jun 21, 2017

View reviewed changes

filmor reviewed Jun 21, 2017

View reviewed changes

pythonnet deleted a comment from den-run-ai Jun 21, 2017

Rickard Holmberg added 3 commits June 21, 2017 12:51

Don't leak init reference

4d50805

Rename tests

b29ae4d

Remove unused internal Runtime.GetBoundArgTuple

f72c8cc

rickardraysearch force-pushed the fix-init branch from 4fe93c4 to f72c8cc Compare June 21, 2017 17:18

Reenable skipped tests in test_subclass.py

db5f43e

den-run-ai requested review from filmor, tonyroberts and vmuriart June 25, 2017 17:27

filmor approved these changes Jun 26, 2017

View reviewed changes

filmor merged commit a5f5dc2 into pythonnet:master Jun 26, 2017

rickardraysearch deleted the fix-init branch June 26, 2017 10:34

filmor mentioned this pull request Jul 11, 2017

Constructor executed twice when overriding constructor in System.Object. #495

Closed

testrunner123 mentioned this pull request Sep 15, 2017

Python grandchild of .NET class is not created correctly if "__namespace__" attribute is set #544

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Another shot at #495 #503

Another shot at #495 #503

rickardraysearch commented Jun 21, 2017 •

edited

Loading

mention-bot commented Jun 21, 2017

codecov bot commented Jun 21, 2017 •

edited

Loading

den-run-ai Jun 21, 2017

filmor Jun 21, 2017

rickardraysearch Jun 21, 2017 •

edited

Loading

filmor commented Jun 21, 2017 •

edited

Loading

rickardraysearch commented Jun 21, 2017

rickardraysearch commented Jun 22, 2017 •

edited

Loading

filmor commented Jun 26, 2017

rickardraysearch commented Jun 26, 2017 •

edited

Loading

filmor commented Jun 26, 2017

Another shot at #495 #503

Another shot at #495 #503

Conversation

rickardraysearch commented Jun 21, 2017 • edited Loading

What does this implement/fix? Explain your changes.

Does this close any currently open issues?

Any other comments?

Checklist

mention-bot commented Jun 21, 2017

codecov bot commented Jun 21, 2017 • edited Loading

Codecov Report

den-run-ai Jun 21, 2017

Choose a reason for hiding this comment

filmor Jun 21, 2017

Choose a reason for hiding this comment

rickardraysearch Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

filmor commented Jun 21, 2017 • edited Loading

rickardraysearch commented Jun 21, 2017

rickardraysearch commented Jun 22, 2017 • edited Loading

filmor commented Jun 26, 2017

rickardraysearch commented Jun 26, 2017 • edited Loading

filmor commented Jun 26, 2017

rickardraysearch commented Jun 21, 2017 •

edited

Loading

codecov bot commented Jun 21, 2017 •

edited

Loading

rickardraysearch Jun 21, 2017 •

edited

Loading

filmor commented Jun 21, 2017 •

edited

Loading

rickardraysearch commented Jun 22, 2017 •

edited

Loading

rickardraysearch commented Jun 26, 2017 •

edited

Loading