bpo-26103 resolve Data descriptor contradiction by aaronchall · Pull Request #1959 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation20 Commits20 Checks0 Files changed
Conversation
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
[ Show hidden characters]({{ revealButtonHref }})
…l/cpython into datadescrcontradiction
must add tests
| class TestIsDataDescriptor(unittest.TestCase): |
|---|
| def test_custom_descriptors(self): |
| class NonDataDescriptor(object): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be backported to 2, it is ostensibly fixing an incorrect implementation.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep the Python 3 code clear. Add the object class in a backport to Python 2.
As it currently stands I am perfectly happy with this revision, but I am more than willing to consider change requests.
| class TestIsDataDescriptor(unittest.TestCase): |
|---|
| def test_custom_descriptors(self): |
| class NonDataDescriptor(object): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep the Python 3 code clear. Add the object class in a backport to Python 2.
| def test_functions(self): |
| class Test(object): |
| def instance_method(self): '' |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use pass for empty body.
| a_lambda = lambda: '' |
|---|
| self.assertFalse(inspect.isdatadescriptor(type(Test().instance_method)), |
| 'a instance method is not a data descriptor') |
| self.assertFalse(inspect.isdatadescriptor(classmethod), |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you test classmethod? Note that the type of class method is not classmethod.
| 'a instance method is not a data descriptor') |
|---|
| self.assertFalse(inspect.isdatadescriptor(classmethod), |
| 'a class method is not a data descriptor') |
| self.assertFalse(inspect.isdatadescriptor(staticmethod), |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
| @@ -1077,6 +1077,51 @@ class C(metaclass=M): |
|---|
| attrs = [a[0] for a in inspect.getmembers(C)] |
| self.assertNotIn('missing', attrs) |
| class TestIsDataDescriptor(unittest.TestCase): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests are not ran. Add the test class in run_unittest() list below.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
| def __delete__(self, name): "" |
|---|
| class DataDescriptor2(object): |
| __set__ = None |
| self.assertFalse(inspect.isdatadescriptor(NonDataDescriptor), |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This returns False because NonDataDescriptor is a class.
| __set__ = None |
|---|
| self.assertFalse(inspect.isdatadescriptor(NonDataDescriptor), |
| 'class with only __get__ not a data descriptor') |
| self.assertTrue(inspect.isdatadescriptor(DataDescriptor0), |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test would fail if TestIsDataDescriptor run.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.
Once you have made the requested changes, please leave a comment on this pull request containing the phrase I didn't expect the Spanish Inquisition!. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
modified: test_inspect.py
I thought it was, "I didn't expect a kind of Spanish Inquisition!" not, I didn't expect the Spanish Inquisition!
(Nevertheless, I appreciate rigor in modifications to the best language of all time!)
Also, I have no idea how to resolve bedevere/news or appveyor's issues...
@serhiy-storchaka Do you want an entry in the Misc/NEWS.d directory? If I do that, should I remove the entry in Misc/NEWS?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @aaronchall! LGTM. But since this is related to delicate matters, it would be nice if yet one core dev will take a look.
In general, this seems fine. My one nit is that rather than "and/or", let's just say "or". "and/or" is jargony.
@benjaminp done, however, the "appveyor" build failed - not sure why, appears to be irrelevant.
Reviewers
