GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-26103 resolve Data descriptor contradiction by aaronchall · Pull Request #1959 · python/cpython

Conversation

@aaronchall

@aaronchall aaronchall commented Jun 5, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

https://bugs.python.org/issue26103

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
aaronchall added 2 commits June 5, 2017 17:47
@aaronchall
update documentation
d9fd529
@aaronchall
update inspect function
42a007b
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Jun 5, 2017
@mention-bot

mention-bot commented Jun 5, 2017

Copy link
Copy Markdown

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aaronchall added 9 commits June 6, 2017 12:01
@aaronchall
add Misc/NEWS entry for abc.ABC slots
7d06227
@aaronchall
add Misc/NEWS entry for inspect.isdatadescriptor
aada79a
@aaronchall
Merge branch 'datadescrcontradiction' of https://github.com/aaronchal…
7164f94 
…l/cpython into datadescrcontradiction
@aaronchall
Merge branch 'master' into datadescrcontradiction
0a993a1
@aaronchall
add tests for isdatadescriptor custom, slots, property and functions
6aa2d87
@aaronchall
Merge branch 'datadescrcontradiction' of https://github.com/aaronchal…
dbc2c25 
…l/cpython into datadescrcontradiction

must add tests
@aaronchall
add self to Misc/ACKS
0858e03
@aaronchall
be more specific in test msg
2439277
@aaronchall
clarify docstring
4bc76c9
matrixise
matrixise reviewed Jun 7, 2017
View reviewed changes
Comment thread Lib/test/test_inspect.py Outdated
class TestIsDataDescriptor(unittest.TestCase):

def test_custom_descriptors(self):
class NonDataDescriptor(object):

@matrixise matrixise Jun 7, 2017

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@aaronchall aaronchall Jun 7, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@serhiy-storchaka serhiy-storchaka Sep 12, 2017

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
aaronchall added 2 commits June 7, 2017 11:33
@aaronchall
clarify "or" is not exclusive
ef3d4d3
@aaronchall
remove "either" - "or" is not exclusive
e2f767c
@aaronchall

aaronchall commented Jun 7, 2017

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka
serhiy-storchaka requested changes Sep 12, 2017
View reviewed changes
Comment thread Lib/test/test_inspect.py Outdated
class TestIsDataDescriptor(unittest.TestCase):

def test_custom_descriptors(self):
class NonDataDescriptor(object):

@serhiy-storchaka serhiy-storchaka Sep 12, 2017

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_inspect.py Outdated

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

@serhiy-storchaka serhiy-storchaka Sep 12, 2017

Copy link
Copy Markdown
Member

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.

Use pass for empty body.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_inspect.py Outdated
a_lambda = lambda: ''
self.assertFalse(inspect.isdatadescriptor(type(Test().instance_method)),
'a instance method is not a data descriptor')
self.assertFalse(inspect.isdatadescriptor(classmethod),

@serhiy-storchaka serhiy-storchaka Sep 12, 2017

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_inspect.py Outdated
'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 serhiy-storchaka Sep 12, 2017

Copy link
Copy Markdown
Member

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.

Ditto.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_inspect.py
attrs = [a[0] for a in inspect.getmembers(C)]
self.assertNotIn('missing', attrs)

class TestIsDataDescriptor(unittest.TestCase):

@serhiy-storchaka serhiy-storchaka Sep 12, 2017

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@aaronchall aaronchall Sep 19, 2017

Copy link
Copy Markdown
Contributor Author

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.

Added

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_inspect.py Outdated
def __delete__(self, name): ""
class DataDescriptor2(object):
__set__ = None
self.assertFalse(inspect.isdatadescriptor(NonDataDescriptor),

@serhiy-storchaka serhiy-storchaka Sep 12, 2017

Copy link
Copy Markdown
Member

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.

This returns False because NonDataDescriptor is a class.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_inspect.py Outdated
__set__ = None
self.assertFalse(inspect.isdatadescriptor(NonDataDescriptor),
'class with only __get__ not a data descriptor')
self.assertTrue(inspect.isdatadescriptor(DataDescriptor0),

@serhiy-storchaka serhiy-storchaka Sep 12, 2017

Copy link
Copy Markdown
Member

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.

This test would fail if TestIsDataDescriptor run.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot

bedevere-bot commented Sep 12, 2017

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting changes label Sep 12, 2017
aaronchall and others added 3 commits September 13, 2017 22:09
@aaronchall
Enabled tests, tests passing, style fixes per request by Serhiy.
2ab592f 
	modified:   test_inspect.py
@aaronchall
Resolving conflict - learned updates go on top
2b0beab
@aaronchall
when in Rome (change from issue to bpo-)
2f3ef81
@aaronchall

aaronchall commented Sep 19, 2017

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting changes label Sep 19, 2017
@bedevere-bot

bedevere-bot commented Sep 19, 2017

Copy link
Copy Markdown

Nobody expects the Spanish Inquisition!

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting change review label Sep 19, 2017
@aaronchall

aaronchall commented Sep 20, 2017

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka

serhiy-storchaka commented Sep 21, 2017

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aaronchall
remove entry - such entries go in News.d now
ed7caa7
@aaronchall
add blurb
eb882ce
serhiy-storchaka
serhiy-storchaka approved these changes May 14, 2018
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting change review labels May 14, 2018
@serhiy-storchaka
serhiy-storchaka requested review from 1st1, benjaminp and rhettinger May 14, 2018 17:22
@benjaminp

benjaminp commented May 15, 2018

Copy link
Copy Markdown
Contributor

In general, this seems fine. My one nit is that rather than "and/or", let's just say "or". "and/or" is jargony.
aaronchall reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aaronchall added 2 commits May 20, 2018 12:20
@aaronchall
change "and/or" to "or"
a69a525
@aaronchall
docstring - change "and/or" to "or"
a7af9f6
@aaronchall

aaronchall commented May 20, 2018

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka
serhiy-storchaka merged commit 4054b17 into python:master May 20, 2018
@bedevere-bot bedevere-bot removed the awaiting merge label May 20, 2018
@hongweipeng hongweipeng mentioned this pull request Oct 8, 2019
bpo-26103: Update the description of isdatadescriptor in inspect.rst #16645
Merged
@aaronchall 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 zuo mentioned this pull request Jul 26, 2024
gh-122102: Fix/improve docs of descriptor-related tools in inspect #122104
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes
@benjaminp benjaminp Awaiting requested review from benjaminp
@1st1 1st1 Awaiting requested review from 1st1
@rhettinger rhettinger Awaiting requested review from rhettinger
+1 more reviewer
@matrixise matrixise matrixise left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@aaronchall @mention-bot @bedevere-bot @serhiy-storchaka @benjaminp @matrixise @the-knights-who-say-ni
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.