bpo-26103 resolve Data descriptor contradiction #1959

aaronchall · 2017-06-05T22:57:47Z

https://bugs.python.org/issue26103

mention-bot · 2017-06-05T22:57:51Z

@aaronchall, thanks for your PR! By analyzing the history of the files in this pull request, we identified @zestyping, @ethanfurman and @birkenfeld to be potential reviewers.

…l/cpython into datadescrcontradiction

…l/cpython into datadescrcontradiction must add tests

matrixise · 2017-06-07T15:32:40Z

Lib/test/test_inspect.py

+class TestIsDataDescriptor(unittest.TestCase):
+
+    def test_custom_descriptors(self):
+        class NonDataDescriptor(object):


Why do you specify the object class for the parent ? it is not required https://docs.python.org/3/reference/compound_stmts.html#class-definitions

This could be backported to 2, it is ostensibly fixing an incorrect implementation.

Keep the Python 3 code clear. Add the object class in a backport to Python 2.

aaronchall · 2017-06-07T17:11:49Z

As it currently stands I am perfectly happy with this revision, but I am more than willing to consider change requests.

serhiy-storchaka · 2017-09-12T18:18:39Z

Lib/test/test_inspect.py

+class TestIsDataDescriptor(unittest.TestCase):
+
+    def test_custom_descriptors(self):
+        class NonDataDescriptor(object):


Keep the Python 3 code clear. Add the object class in a backport to Python 2.

serhiy-storchaka · 2017-09-12T18:19:57Z

Lib/test/test_inspect.py

+
+    def test_functions(self):
+        class Test(object):
+            def instance_method(self): ''


Use pass for empty body.

serhiy-storchaka · 2017-09-12T18:23:19Z

Lib/test/test_inspect.py

+        a_lambda = lambda: ''
+        self.assertFalse(inspect.isdatadescriptor(type(Test().instance_method)),
+                         'a instance method is not a data descriptor')
+        self.assertFalse(inspect.isdatadescriptor(classmethod),


Why you test classmethod? Note that the type of class method is not classmethod.

serhiy-storchaka · 2017-09-12T18:23:34Z

Lib/test/test_inspect.py

+                         '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),


serhiy-storchaka · 2017-09-12T18:30:59Z

Lib/test/test_inspect.py

        attrs = [a[0] for a in inspect.getmembers(C)]
        self.assertNotIn('missing', attrs)

+class TestIsDataDescriptor(unittest.TestCase):


These tests are not ran. Add the test class in run_unittest() list below.

serhiy-storchaka · 2017-09-12T18:31:07Z

Lib/test/test_inspect.py

+            def __delete__(self, name): ""
+        class DataDescriptor2(object):
+            __set__ = None
+        self.assertFalse(inspect.isdatadescriptor(NonDataDescriptor),


This returns False because NonDataDescriptor is a class.

serhiy-storchaka · 2017-09-12T18:32:06Z

Lib/test/test_inspect.py

+            __set__ = None
+        self.assertFalse(inspect.isdatadescriptor(NonDataDescriptor),
+                         'class with only __get__ not a data descriptor')
+        self.assertTrue(inspect.isdatadescriptor(DataDescriptor0),


This test would fail if TestIsDataDescriptor run.

bedevere-bot · 2017-09-12T18:43:24Z

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

aaronchall · 2017-09-19T15:36:42Z

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...

bedevere-bot · 2017-09-19T15:36:44Z

Nobody expects the Spanish Inquisition!

@serhiy-storchaka: please review the changes made to this pull request.

aaronchall · 2017-09-20T00:19:07Z

@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?

serhiy-storchaka · 2017-09-21T20:38:34Z

Misc/NEWS no longer edited directly. You should add new entries into the Misc/NEWS.d directory as individual files. See https://docs.python.org/devguide/committing.html#what-s-new-and-news-entries.

serhiy-storchaka

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.

benjaminp · 2018-05-15T03:46:57Z

In general, this seems fine. My one nit is that rather than "and/or", let's just say "or". "and/or" is jargony.

aaronchall · 2018-05-20T18:44:59Z

@benjaminp done, however, the "appveyor" build failed - not sure why, appears to be irrelevant.

aaronchall added 2 commits June 5, 2017 17:47

update documentation

d9fd529

update inspect function

42a007b

the-knights-who-say-ni added the CLA signed label Jun 5, 2017

aaronchall added 9 commits June 6, 2017 12:01

add Misc/NEWS entry for abc.ABC slots

7d06227

add Misc/NEWS entry for inspect.isdatadescriptor

aada79a

Merge branch 'datadescrcontradiction' of https://github.com/aaronchal…

7164f94

…l/cpython into datadescrcontradiction

Merge branch 'master' into datadescrcontradiction

0a993a1

add tests for isdatadescriptor custom, slots, property and functions

6aa2d87

Merge branch 'datadescrcontradiction' of https://github.com/aaronchal…

dbc2c25

…l/cpython into datadescrcontradiction must add tests

add self to Misc/ACKS

0858e03

be more specific in test msg

2439277

clarify docstring

4bc76c9

matrixise reviewed Jun 7, 2017

View reviewed changes

aaronchall added 2 commits June 7, 2017 11:33

clarify "or" is not exclusive

ef3d4d3

remove "either" - "or" is not exclusive

e2f767c

serhiy-storchaka requested changes Sep 12, 2017

View reviewed changes

bedevere-bot added the awaiting changes label Sep 12, 2017

aaronchall and others added 3 commits September 13, 2017 22:09

Enabled tests, tests passing, style fixes per request by Serhiy.

2ab592f

modified: test_inspect.py

Resolving conflict - learned updates go on top

2b0beab

when in Rome (change from issue to bpo-)

2f3ef81

bedevere-bot removed the awaiting changes label Sep 19, 2017

bedevere-bot added the awaiting change review label Sep 19, 2017

remove entry - such entries go in News.d now

ed7caa7

add blurb

eb882ce

serhiy-storchaka approved these changes May 14, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels May 14, 2018

serhiy-storchaka requested review from 1st1, benjaminp and rhettinger May 14, 2018 17:22

aaronchall added 2 commits May 20, 2018 12:20

change "and/or" to "or"

a69a525

docstring - change "and/or" to "or"

a7af9f6

serhiy-storchaka merged commit 4054b17 into python:master May 20, 2018

bedevere-bot removed the awaiting merge label May 20, 2018

hongweipeng mentioned this pull request Oct 8, 2019

bpo-26103: Update the description of isdatadescriptor in inspect.rst #16645

Merged

aaronchall mannequin mentioned this pull request Apr 6, 2023

Contradiction in definition of "data descriptor" between (dotted lookup behavior/datamodel documentation) and (inspect lib/descriptor how-to) #70291

Closed

zuo mentioned this pull request Jul 26, 2024

gh-122102: Fix/improve docs of descriptor-related tools in inspect #122104

Open

Uh oh!

bpo-26103 resolve Data descriptor contradiction #1959

bpo-26103 resolve Data descriptor contradiction #1959

Uh oh!

Conversation

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

Uh oh!

Uh oh!

mention-bot commented Jun 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronchall Jun 7, 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!

Choose a reason for hiding this comment

Uh oh!

aaronchall commented Jun 7, 2017

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!

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!

bedevere-bot commented Sep 12, 2017

Uh oh!

aaronchall commented Sep 19, 2017

Uh oh!

bedevere-bot commented Sep 19, 2017

Uh oh!

aaronchall commented Sep 20, 2017

Uh oh!

serhiy-storchaka commented Sep 21, 2017

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

benjaminp commented May 15, 2018

Uh oh!

aaronchall commented May 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

aaronchall commented Jun 5, 2017 •

edited

Loading

aaronchall Jun 7, 2017 •

edited

Loading