Provide hook to implement repr by koubaa · Pull Request #808 · pythonnet/pythonnet · GitHub

Issue: 680

What does this implement/fix? Explain your changes.

This allows C# types to provide __repr__ implementations

Does this close any currently open issues?

680

Any other comments?

Please confirm my expections in the test I added. Inheritance hierarchies in particular were tricky to get right.

Checklist

Check all those that are applicable and complete.

Make sure to include one or more tests for your change
If an enhancement PR, please create docs and at best an example
Add yourself to AUTHORS
Updated the CHANGELOG

Codecov Report

Merging #808 into master will decrease coverage by 0.28%.
The diff coverage is 55.26%.

@@            Coverage Diff             @@
##           master     #808      +/-   ##
==========================================
- Coverage   76.91%   76.62%   -0.29%     
==========================================
  Files          64       64              
  Lines        5935     5973      +38     
  Branches      976      986      +10     
==========================================
+ Hits         4565     4577      +12     
- Misses       1040     1059      +19     
- Partials      330      337       +7

Flag	Coverage Δ
#setup_linux	65.3% <ø> (ø)	⬆️
#setup_windows	75.85% <55.26%> (-0.29%)	⬇️

Impacted Files	Coverage Δ
src/runtime/classbase.cs	74.8% <55.17%> (-6.57%)	⬇️
src/runtime/exceptions.cs	79.03% <55.55%> (-1.84%)	⬇️
src/runtime/delegatemanager.cs	81.96% <0%> (-4.92%)	⬇️
src/runtime/pythonexception.cs	76.66% <0%> (-3.34%)	⬇️

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 868cd52...82aea1f. Read the comment docs.

@koubaa one error here:

=================================== ERRORS ====================================
___________________ ERROR collecting src/tests/test_repr.py ___________________
ImportError while importing test module 'C:\projects\pythonnet\src\tests\test_repr.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
src\tests\test_repr.py:7: in <module>
    from Python.Test import (ReprTest)
E   ImportError: cannot import name 'ReprTest' from 'Python.Test' (unknown location)
!!!!!!!!!!!!!!!!!!! Interrupted: 1 errors during collection !!!!!!!!!!!!!!!!!!!

@denfromufa How do I run the tests locally? I want to make sure they work before pushing. (I just tested manually in the console and tried to follow the pattern)

I figured it out (took longer than I'll admit!)

One test fails because the repr of System.OverflowException went from
OverflowException('Simple message..."
to
<System.OverflowException object at 0xADDRESS>

I'll need to figure out how the old behavior was working to see why its broken now.

@koubaa were you able to run the tests locally? If not, then I can search if we have instructions somewhere.

@denfromufa yes I can run locally now. I have vs2017 installed and ran this to build:

python setup.py install --xplat

then this to test:

python -m pytest

Should this be in the README?

This is only partial tests, there are also embedding tests run through nunit. Documentation improvements are welcome, probably this one should go into wiki.

…

On Tue, Jan 22, 2019, 6:43 PM Mohamed Koubaa ***@***.***> wrote: @denfromufa <https://github.com/denfromufa> yes I can run locally now. I have vs2017 installed and ran this to build: python setup.py install --xplat then this to test: python -m pytest Should this be in the README? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#808 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHgZ5RkZgmBsugyNIIRfmhr349PPwytMks5vF7BCgaJpZM4aJb9z> .

@denfromufa sounds good. I didn't see any embedded test fails in previous CI runs so I'm OK running these partial tests for now.

I tried to find where OverflowException repr is coming from (on master) and actually no breakpoints in tp_repr defined were hit. So I really don't know (1) how else to look for it and (2) how I could test that its an exception in classbase tp_repr and fallback to python defaults.

@denfromufa I am back from a vacation so I looked at this again. I found this change which removed tp_repr from ExceptionClassObject. After some trial and error I discovered that __repr__() works on master because of what's done in SetArgsAndCause and the fact that a __repr__() was not defined. Now that ClassBase implements tp_repr, the CPython runtime will not fall back to the args/cause which were set.

My plan is to re-implement repr in ExceptionClassObject as was done before that change. Please let me know if you have any concerns with this approach.

This is only partial tests, there are also embedding tests run through nunit. Documentation improvements are welcome, probably this one should go into wiki.

I just edited the wiki with what I know about so far.

python tests now pass in CI. Embedded tests pass locally (on windows) but not in CI, so I'm adding some logging statements to see where.

Thanks already for your contribution. I have to think a bit about whether the "new" __str__ behaviour is how I would expect things to work.

Issue: 680

I'm having a lot of trouble with these CI tests.

The latest Travis job is not using my latest changes. Appveyor at least reports the correct SHA1 but have the below errors.
Appveyor 2.7 x64 xplat hangs for one hour when about to run embedded tests.
Appveyor 3.4 x86 xplat fails in pip install
Appveyor 3.4 x64 xplat also fails in pip install
Appveyor 2.7 x86 seems to have blown up because of the print statements I added. Is there a reason why Console.WriteLine is crashing (i.e. is stdout unavailable?) and if so what's the preferred way to add this sort of debugging logging statements?
Appveyor 2.7 x64 complains of a test failure but I cannot find much more details.
Appveyor 3.4 x86 also fails in setuptools
Appveyor 3.4 x64 also fails in setuptools.

Since all of the appveyor py3.4 are failing in setuptools it seems to me that its fallen out of maintenance. The 1 hour hangs concern me most since everything is queued and it delays other tests.

@filmor we can probably make python 3.4 optional in CI tests as a temporary solution until we drop it?

I'll have a look into this this weekend. Could you please remove the debug output?

done

Seems better but its hard to determine what's wrong with x64 2.7. It says python tests failed but no indication which ones.

From the logs I think that the issue is aimply the casing of the ReprTests.cs. In the repository it's written in camel case, while in the csproj file ot's reprtests.cs. Correct the csproj and it should go through.

@filmor done. Hopefully this works. Do you agree with the new str behavior()?

Each dot indicates a successful test, so you can just count ;)

I'll have a look into this as soon as I have finally finished the 2.4.0 release.

Codecov Report

Merging #808 into master will decrease coverage by 2.65%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #808      +/-   ##
==========================================
- Coverage   86.71%   84.05%   -2.66%     
==========================================
  Files           1        1              
  Lines         301      301              
==========================================
- Hits          261      253       -8     
- Misses         40       48       +8

Flag	Coverage Δ
#setup_linux	65.44% <ø> (ø)	⬆️
#setup_windows	65.78% <ø> (-5.65%)	⬇️

Impacted Files	Coverage Δ
setup.py	84.05% <0%> (-2.66%)	⬇️

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 60e6045...264c78b. Read the comment docs.

Looks like the problems were magically fixed :)

Since all .NET types have ToString, why does it even try __repr__? If somebody really needs it, they can do override string ToString() => __repr__()

That's not the same, though. ToString is more akin to __str__, there is no real equivalent to __repr__ in .NET.

Yeah, this is the tp_str implementation. tp_repr is below

@lostmsu See the comment:
//The return value must be a string object. If a class defines repr() but not str(),
//then repr() is also used when an “informal” string representation of instances of that
//class is required.

This sentence comes straight from the python manual, I didn't make it up

@koubaa the default inheritance hierarchy for C# classes from Python's point of view is Python's object <- System.Object <- CustomCSharpClass.

Since for all .NET classes overriding ToString is how you implement __str__, I don't see why it should be different for System.Object. Hence I think .NET classes should just always call ToString for tp_str.

@lostmsu I took a look. I agree some kind of consensus between interface vs reflection for dunder methods should be reached and applied throughout the code base. One argument for reflection is that you can use extension methods to add these. If I take an existing C# codebase and write some extension methods in a separate assembly, I can provide __repr__ and __getattr__ to it. What do you think?

@koubaa, extension methods would be hard to find. You'd have to enumerate all types in all assemblies. GetMethods won't return them.

In addition to that, what would be the semantics of multiple assemblies defining an extension method with the same signature?

@lostmsu ah, didn't know that.

@lostmsu I am still not convinced about the interface approach. It would require a hard build dependency on C# libraries onto PythonNet. It seems like a tough sell for some of the mature C# libraries out there.

@lostmsu I changed this as you requested. I don't think I can capture the repr function in a Func object since this is a static method.

Yeah, this is the tp_str implementation. tp_repr is below

Looks like the problems were magically fixed :)

This is equal parts annoying and relieving

@lostmsu See the comment:
//The return value must be a string object. If a class defines repr() but not str(),
//then repr() is also used when an “informal” string representation of instances of that
//class is required.

This sentence comes straight from the python manual, I didn't make it up

To help you in disagreeing, I propose the another style: We add a new generic abstract class (or interface with default methods when we convert to C#8) PythonWrapper and allow it to be registered for automatic conversion:

using SomeLib;

[Python.Conversion]
public class SomeClassWrapper : PythonWrapper<SomeClass> {
    public string Repr() => $"<SomeClass {this.wrapped.Name}>";
}

Now this means that we would need to link Python.Runtime.dll (or a much smaller DLL that is only used for these markers and the interface) into the wrapper DLL, but this is not an issue in embedding situations and everywhere else we could use the mentioned "smaller", platform-independent DLL.

@filmor not sure I understood your proposal.
Can you give a more complete example? What should PythonWrapper<T> do?

Hey, can this go in yet?

It can, thanks a lot for your contribution :)

Decreasing args reference count actually decreases ob reference count, which marks it as garbage for the GC!
After a while GC frees the object which causes the program to crash if the object is used again.
One fix for this can be adding the line:
Runtime.XIncref(ob);
at line 269 (before setting ob to the tuple's first argument).

Thanks a lot for catching this. Would you prepare a small PR?

Done
1160

Provide hook to implement __repr__

koubaa mentioned this pull request Jan 20, 2019

How to implement __repr__ for a C# class to be used in python.net? #680

Closed

koubaa force-pushed the repr branch 2 times, most recently from 859f80b to 8f6bdce Compare February 5, 2019 01:30

koubaa force-pushed the repr branch from 8f6bdce to 48e3077 Compare February 5, 2019 01:57

filmor added this to the 2.4.0 milestone Feb 5, 2019

filmor reviewed Feb 5, 2019

View reviewed changes

Comment thread src/runtime/classbase.cs Outdated

Comment thread src/runtime/classbase.cs Outdated

Comment thread src/runtime/classbase.cs Outdated

Provide hook to implement __repr__

351d9ff

Issue: 680

koubaa force-pushed the repr branch from e093acd to 107d22f Compare February 10, 2019 22:07

koubaa force-pushed the repr branch from e5fe9a2 to 351d9ff Compare February 27, 2019 22:54

Merge branch 'master' into repr

d71a391

fix casing

2a002d7

filmor modified the milestones: 2.4.0, 2.4.1 Apr 12, 2019

filmor added 3 commits June 20, 2019 19:32

Merge branch 'master' into repr

502f0c3

Merge branch 'master' into repr

eb4004a

Merge branch 'master' into repr

c6ae6a4

filmor added the next label Sep 12, 2019

lostmsu requested changes Sep 12, 2019

View reviewed changes

lostmsu requested changes Sep 14, 2019

View reviewed changes

koubaa added 2 commits September 15, 2019 13:32

update based on review

0916406

fix whitespace

2fb6a43

koubaa commented Sep 15, 2019

View reviewed changes

koubaa added 2 commits September 15, 2019 13:54

fix whitespace

9fc5b5f

fix

00b55db

amos402 reviewed Sep 24, 2019

View reviewed changes

Comment thread src/runtime/classbase.cs Outdated

koubaa added 3 commits October 16, 2019 17:56

Don't use repr in __str__. Clean up unneeded tests

accc797

fix leak

ff5d96c

Merge branch 'master' into repr

264c78b

lostmsu approved these changes Oct 17, 2019

View reviewed changes

filmor merged commit 4a9457f into pythonnet:master Oct 18, 2019

lostmsu modified the milestones: 2.4.1, 2.5.0 Apr 23, 2020

DanBarzilian reviewed Jun 16, 2020

View reviewed changes

AlexCatarino pushed a commit to QuantConnect/pythonnet that referenced this pull request Jun 27, 2020

Provide hook to implement __repr__ (pythonnet#808 )

50ccbfd

Provide hook to implement __repr__

+                              Runtime.PyTuple_SetItem(args, 0, ob);
+                              IntPtr reprFunc = Runtime.PyObject_GetAttrString(Runtime.PyBaseObjectType, "__repr__");
+                              var output =  Runtime.PyObject_Call(reprFunc, args, IntPtr.Zero);
+                              Runtime.XDecref(args);

+                              }
+                              //If the object defines __repr__, call it.
+                              System.Reflection.MethodInfo reprMethodInfo = instType.GetMethod("__repr__");

Provide hook to implement __repr__ by koubaa · Pull Request #808 · pythonnet/pythonnet · GitHub

Uh oh!

Provide hook to implement __repr__ - #808

Provide hook to implement __repr__#808filmor merged 17 commits intopythonnet:masterpythonnet/pythonnet:masterfrom koubaa:reprkoubaa/pythonnet:reprCopy head branch name to clipboard

Conversation

koubaa commented Jan 20, 2019 • 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!

codecov Bot commented Jan 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

den-run-ai commented Jan 21, 2019

Uh oh!

koubaa commented Jan 21, 2019

Uh oh!

koubaa commented Jan 22, 2019

Uh oh!

koubaa commented Jan 22, 2019

Uh oh!

den-run-ai commented Jan 22, 2019

Uh oh!

koubaa commented Jan 23, 2019

Uh oh!

den-run-ai commented Jan 23, 2019 via email

Uh oh!

koubaa commented Jan 23, 2019

Uh oh!

koubaa commented Feb 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

koubaa commented Feb 5, 2019

Uh oh!

koubaa commented Feb 5, 2019

Uh oh!

filmor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

koubaa commented Feb 11, 2019

Uh oh!

den-run-ai commented Feb 21, 2019

Uh oh!

filmor commented Feb 25, 2019

Uh oh!

koubaa commented Feb 27, 2019

Uh oh!

koubaa commented Mar 7, 2019

Uh oh!

filmor commented Mar 8, 2019

Uh oh!

koubaa commented Mar 8, 2019

Uh oh!

filmor commented Apr 8, 2019

Uh oh!

codecov-io commented Sep 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

filmor commented Sep 12, 2019

Uh oh!

lostmsu Sep 12, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koubaa Sep 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Provide hook to implement repr by koubaa · Pull Request #808 · pythonnet/pythonnet · GitHub

Provide hook to implement repr - #808

Provide hook to implement repr#808
filmor merged 17 commits into
pythonnet:masterfrom
koubaa:repr

koubaa commented Jan 20, 2019 •
edited
Loading

codecov Bot commented Jan 20, 2019 •
edited
Loading

koubaa commented Feb 4, 2019 •
edited
Loading

codecov-io commented Sep 12, 2019 •
edited
Loading

lostmsu Sep 12, 2019 •
edited
Loading

koubaa Sep 16, 2019 •
edited
Loading