Minor follow-on to PR #1334 (Fix enum value's __int__ returning non-int when underlying type is bool or of char type)#3232
Conversation
…he comment and empty line that was added in PR pybind#3087; those were made obsolete by the pragma cleanup that concluded with PR pybind#3186.
|
@Vigilans Could you please take a look at these tweaks and approve it it looks good to you, or let me know any suggestions? |
Vigilans
left a comment
There was a problem hiding this comment.
I've written down my original intent of each test/comment to help clarifying the story and help you decide whether they should be removed or tweaked. I agree with your comments and am fine with any change if it is misleading.
|
|
||
| def test_char_underlying_enum(): # Issue #1331/PR #1334: | ||
| assert type(m.ScopedCharEnum.Positive.__int__()) is int | ||
| assert int(m.ScopedChar16Enum.Zero) == 0 # int() call should successfully return |
There was a problem hiding this comment.
The intent of this test was that when __int__() returns a non-int, int() call will throw a error:
>>> Player.black.__int__()
'\x01'
>>> int(Player.black)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: __int__ returned non-int (type str)
So I wrote this call in the test and expected it to successfully return.
There was a problem hiding this comment.
Thanks. I think that is clear just from the test code itself.
tests/test_enum.py
Outdated
| assert int(m.ScopedChar16Enum.Zero) == 0 | ||
| assert hash(m.ScopedChar32Enum.Positive) == 1 | ||
| assert m.ScopedCharEnum.Positive.__getstate__() == 1 # return type is long in py2.x | ||
| assert m.ScopedCharEnum.Positive.__getstate__() == 1 |
There was a problem hiding this comment.
Yes, the intent of this test is to exercise __getstate__() call (along with __hash__() call). The intent of this comment was because __getstate__() and __hash__() returns a long type in python 2.x, so
assert type(m.ScopedCharEnum.Positive.__getstate__()) is int
will fail in python 2.x platforms. So I tested them by comparing with 1.
There was a problem hiding this comment.
Aha! :-)
I changed this, using if env.PY2, so that we get what you originally had in mind for Python 3.
The if env.PY2 branch will get purged when we clean out Python 2 support in a few months.
Unfortunately our pre-commit checks (black, flake8) get upset about type(...) is long. To not make things overly complicated for Python 2 I just kept your == 1 test for Python 2.
| with pytest.raises(TypeError): | ||
| # Enum should construct with a int, even with char underlying type | ||
| m.ScopedWCharEnum("0") | ||
| # Even if the underlying type is char, only an int can be used to construct the enum: |
There was a problem hiding this comment.
The intent of this test, I remembered, was that when enum's underlying type is char, then pybind11's enum not only returns a string, but also could only be constructed with a string. So I wrote a test here to expect it to throw a TypeError instead of returning successfully.
|
@Vigilans thanks for the clarifications! |
This PR just removes two comments, changes one, and tweaks a test for consistency with a comment.
The main motivation for creating this PR was this comment:
The comment made me wonder if the
.__getstate__()is an accommodation for Python 2 (that we'd need to take care of later when we purge Python 2 support).But replacing it with
int(assert m.ScopedCharEnum.Positive) == 1and running the CI also works on all platforms.Therefore I'm now thinking that the intent was to actually exercise
.__getstate__(). Is that correct?In that case I think the comment is misleading and better removed.
While I was at it:
This comment seems completely redundant (therefore more likely to be confusing than helpful):
I made this change because the original comment seemed unclear to me:
I rewrote it according to my understanding. Is that correct?
While I was at that, I removed the
WinWChar, to make the test match the comment more closely.