Another shot at #495 by rickardraysearch · Pull Request #503 · pythonnet/pythonnet

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

Comments

Another shot at #495#503

Another shot at #495#503
filmor merged 10 commits intopythonnet:masterfrom
rickardraysearch:fix-init

rickardraysearch commented Jun 21, 2017 •

edited

Loading

Uh oh!

mention-bot commented Jun 21, 2017

Uh oh!

codecov bot commented Jun 21, 2017 •

edited

Loading

Uh oh!

den-run-ai Jun 21, 2017

Uh oh!

filmor Jun 21, 2017

Uh oh!

rickardraysearch Jun 21, 2017 •

edited

Loading

Uh oh!

filmor commented Jun 21, 2017 •

edited

Loading

Uh oh!

rickardraysearch commented Jun 21, 2017

Uh oh!

rickardraysearch commented Jun 22, 2017 •

edited

Loading

Uh oh!

filmor commented Jun 26, 2017

Uh oh!

rickardraysearch commented Jun 26, 2017 •

edited

Loading

Uh oh!

filmor commented Jun 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

Conversation

rickardraysearch commented Jun 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this implement/fix? Explain your changes.

Does this close any currently open issues?

Any other comments?

Checklist

Uh oh!

mention-bot commented Jun 21, 2017

Uh oh!

codecov bot commented Jun 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

den-run-ai Jun 21, 2017

Choose a reason for hiding this comment

Uh oh!

filmor Jun 21, 2017

Choose a reason for hiding this comment

Uh oh!

rickardraysearch Jun 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filmor commented Jun 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rickardraysearch commented Jun 21, 2017

Uh oh!

rickardraysearch commented Jun 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

filmor commented Jun 26, 2017

Uh oh!

rickardraysearch commented Jun 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

filmor commented Jun 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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