-
Notifications
You must be signed in to change notification settings - Fork 773
Provide hook to implement __repr__ #808
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 14 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
351d9ff
Provide hook to implement __repr__
koubaa d71a391
Merge branch 'master' into repr
filmor 2a002d7
fix casing
koubaa cf1f1dc
Merge branch 'master' into repr
filmor 1c0984f
Merge branch 'master' into repr
koubaa a5b245b
Merge branch 'master' into repr
filmor 82aea1f
Merge branch 'master' into repr
filmor 502f0c3
Merge branch 'master' into repr
filmor eb4004a
Merge branch 'master' into repr
filmor c6ae6a4
Merge branch 'master' into repr
filmor 0916406
update based on review
koubaa 2fb6a43
fix whitespace
koubaa 9fc5b5f
fix whitespace
koubaa 00b55db
fix
koubaa accc797
Don't use repr in __str__. Clean up unneeded tests
koubaa ff5d96c
fix leak
koubaa 264c78b
Merge branch 'master' into repr
koubaa File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| using System; | ||
| using System.Text; | ||
|
|
||
| namespace Python.Test | ||
| { | ||
| /// <summary> | ||
| /// Supports repr unit tests. | ||
| /// </summary> | ||
| public class ReprTest | ||
| { | ||
| public class Point | ||
| { | ||
| public Point(double x, double y) | ||
| { | ||
| X = x; | ||
| Y = y; | ||
| } | ||
|
|
||
| public double X { get; set; } | ||
| public double Y { get; set; } | ||
|
|
||
| public override string ToString() | ||
| { | ||
| return base.ToString() + ": X=" + X.ToString() + ", Y=" + Y.ToString(); | ||
| } | ||
|
|
||
| public string __repr__() | ||
| { | ||
| return "Point(" + X.ToString() + "," + Y.ToString() + ")"; | ||
| } | ||
| } | ||
|
|
||
| public class Foo | ||
| { | ||
| public string __repr__() | ||
| { | ||
| return "I implement __repr__() but not ToString()!"; | ||
| } | ||
| } | ||
|
|
||
| public class Bar | ||
| { | ||
| public override string ToString() | ||
| { | ||
| return "I implement ToString() but not __repr__()!"; | ||
| } | ||
| } | ||
|
|
||
| public class BazBase | ||
| { | ||
| public override string ToString() | ||
| { | ||
| return "Base class implementing ToString()!"; | ||
| } | ||
| } | ||
|
|
||
| public class BazMiddle : BazBase | ||
| { | ||
| public override string ToString() | ||
| { | ||
| return "Middle class implementing ToString()!"; | ||
| } | ||
| } | ||
|
|
||
| //implements ToString via BazMiddle | ||
| public class Baz : BazMiddle | ||
| { | ||
|
|
||
| } | ||
|
|
||
| public class Quux | ||
| { | ||
| public string ToString(string format) | ||
| { | ||
| return "I implement ToString() with an argument!"; | ||
| } | ||
| } | ||
|
|
||
| public class QuuzBase | ||
| { | ||
| protected string __repr__() | ||
| { | ||
| return "I implement __repr__ but it isn't public!"; | ||
| } | ||
| } | ||
|
|
||
| public class Quuz : QuuzBase | ||
| { | ||
|
|
||
| } | ||
|
|
||
| public class Corge | ||
| { | ||
| public string __repr__(int i) | ||
| { | ||
| return "__repr__ implemention with input parameter!"; | ||
| } | ||
| } | ||
|
|
||
| public class Grault | ||
| { | ||
| public int __repr__() | ||
| { | ||
| return "__repr__ implemention with wrong return type!".Length; | ||
| } | ||
| } | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| # -*- coding: utf-8 -*- | ||
|
|
||
| """Test __repr__ output""" | ||
|
|
||
| import System | ||
| import pytest | ||
| from Python.Test import ReprTest | ||
|
|
||
| def test_basic(): | ||
| """Test Point class which implements both ToString and __repr__ without inheritance""" | ||
| ob = ReprTest.Point(1,2) | ||
| # point implements ToString() and __repr__() | ||
| assert ob.__repr__() == "Point(1,2)" | ||
| assert str(ob) == "Python.Test.ReprTest+Point: X=1, Y=2" | ||
|
|
||
| def test_system_string(): | ||
| """Test system string""" | ||
| ob = System.String("hello") | ||
| assert str(ob) == "hello" | ||
| assert "<System.String object at " in ob.__repr__() | ||
|
|
||
| def test_repr_only(): | ||
| """Test class implementing __repr__() but not ToString()""" | ||
| ob = ReprTest.Foo() | ||
| assert str(ob) == "I implement __repr__() but not ToString()!" | ||
|
koubaa marked this conversation as resolved.
Outdated
|
||
| assert ob.__repr__() == "I implement __repr__() but not ToString()!" | ||
|
|
||
| def test_str_only(): | ||
| """Test class implementing ToString() but not __repr__()""" | ||
| ob = ReprTest.Bar() | ||
| assert str(ob) == "I implement ToString() but not __repr__()!" | ||
| assert "<Python.Test.Bar object at " in ob.__repr__() | ||
|
|
||
| def test_hierarchy1(): | ||
| """Test inheritance heirarchy with base & middle class implementing ToString""" | ||
| ob1 = ReprTest.BazBase() | ||
| assert str(ob1) == "Base class implementing ToString()!" | ||
| assert "<Python.Test.BazBase object at " in ob1.__repr__() | ||
|
|
||
| ob2 = ReprTest.BazMiddle() | ||
| assert str(ob2) == "Middle class implementing ToString()!" | ||
| assert "<Python.Test.BazMiddle object at " in ob2.__repr__() | ||
|
|
||
| ob3 = ReprTest.Baz() | ||
| assert str(ob3) == "Middle class implementing ToString()!" | ||
| assert "<Python.Test.Baz object at " in ob3.__repr__() | ||
|
|
||
| def bad_tostring(): | ||
| """Test ToString that can't be used by str()""" | ||
| ob = ReprTest.Quux() | ||
| assert str(ob) == "Python.Test.ReprTest+Quux" | ||
| assert "<Python.Test.Quux object at " in ob.__repr__() | ||
|
|
||
| def bad_repr(): | ||
| """Test incorrect implementation of repr""" | ||
| ob1 = ReprTest.QuuzBase() | ||
| assert str(ob1) == "Python.Test.ReprTest+QuuzBase" | ||
| assert "<Python.Test.QuuzBase object at " in ob.__repr__() | ||
|
|
||
| ob2 = ReprTest.Quuz() | ||
| assert str(ob2) == "Python.Test.ReprTest+Quuz" | ||
| assert "<Python.Test.Quuz object at " in ob.__repr__() | ||
|
|
||
| ob3 = ReprTest.Corge() | ||
| with pytest.raises(Exception): | ||
| str(ob3) | ||
| with pytest.raises(Exception): | ||
| ob3.__repr__() | ||
|
|
||
| ob4 = ReprTest.Grault() | ||
| with pytest.raises(Exception): | ||
| str(ob4) | ||
| with pytest.raises(Exception): | ||
| ob4.__repr__() | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since all .NET types have
ToString, why does it even try__repr__? If somebody really needs it, they can dooverride string ToString() => __repr__()There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not the same, though.
ToStringis more akin to__str__, there is no real equivalent to__repr__in .NET.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is the
tp_strimplementation.tp_repris belowThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
ToStringis how you implement__str__, I don't see why it should be different forSystem.Object. Hence I think .NET classes should just always callToStringfortp_str.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@koubaa, extension methods would be hard to find. You'd have to enumerate all types in all assemblies.
GetMethodswon't return them.In addition to that, what would be the semantics of multiple assemblies defining an extension method with the same signature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lostmsu ah, didn't know that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.