classification
Title: Make pydoc consistent about bound methods
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: larry Nosy List: larry, ncoghlan, python-dev, yselivanov
Priority: release blocker Keywords: patch

Created on 2014-02-21 00:19 by larry, last changed 2014-03-17 06:30 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
larry.fix.help.on.bound.methods.1.diff larry, 2014-02-21 00:19 review
larry.fix.help.on.bound.methods.2.diff larry, 2014-02-21 00:29 review
larry.fix.help.on.bound.methods.3.diff larry, 2014-02-21 01:28 review
larry.fix.help.on.bound.methods.4.diff larry, 2014-02-21 01:51 review
larry.fix.help.on.bound.methods.5.diff larry, 2014-02-21 03:11 review
larry.fix.help.on.bound.methods.6.diff larry, 2014-02-21 06:52 review
larry.fix.help.on.bound.methods.7.diff larry, 2014-02-21 07:13 review
larry.fix.help.on.bound.methods.8.diff larry, 2014-02-21 07:20 review
Messages (23)
msg211768 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-21 00:19
inspect has a bug:
* inspect.ismethod() returns False for bound methods on builtins.

If I fix that, that exposes a bug in pydoc:
* pydoc's two docroutine() functions assume that bound methods
  have a __func__; bound builtins do not.

The only reason pydoc assumed the __func__ attribute was so it could use that instead of the bound function when getting the signature.  I don't know why it cared, but: when it does so, that means it displays "self" for bound methods implemented in Python.

However, since it doesn't do that for bound methods implemented in C, now the behavior is inconsistent.  Should it display self or not?  The consensus was, it should not.  This would make pydoc consistent with inspect.signature.

This patch therefore changes a third thing:

* pydoc: don't display "self" for bound methods implemented in Python.

I propose to merge this for 3.4.0rc2.
msg211769 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-02-21 00:23
Larry, I think you can use undocumented and private (but still heavily tested) 'inspect._signature_internal(skip_bound_arg=False)', instead of signature.  That will give you a Signature object with 'self' parameter always included.

And in 3.5 we'll probably make this option (renamed, hopefully), public, and switch pydoc to use new API again.

What do you think?
msg211770 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-02-21 00:25
(The thing with my proposal, is that pydoc will handle everything that 'inspect.signature' supports + it's behaviour won't change at all)
msg211771 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-21 00:29
Whoops, the test suite hadn't finished when I posted that, and it had an error.  Here's an updated patch that fixes the tests.

Also, there's one more minor fix in the patch.  pydoc has cut-and-pasted code for HTML versus text rendering, and they had fallen out of sync.  This patch brings them into sync.
msg211772 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-02-21 00:36
I don't think we should touch 'inspect.isbuiltin' at this stage.
Please consider my proposal instead.
msg211773 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-21 00:37
We discussed it in python-dev:
  https://mail.python.org/pipermail/python-dev/2014-January/132051.html

And the overwhelming majority voted +1 for "don't show self for bound methods".  So I'd like to change it for 3.4.  (I meant to get it in earlier but lacked the time.)
msg211774 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-21 00:38
I'm not proposing to modify isbuiltin.  Rather, I'm proposing to fix a bug in ismethod.
msg211775 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-02-21 00:41
> Rather, I'm proposing to fix a bug in ismethod.
Oh, sorry, that's what I meant. I don't think it's good to fix it either.

But please hold on, I think I found a bug in inspect.signature.
msg211777 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-02-21 00:54
Indeed, here it is: #20711
msg211778 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-21 00:58
So why is it necessary (and a release blocker) to fix a bug in inspect._signature_fromstr, but a terrible idea to fix a bug in inspect.ismethod?
msg211779 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-02-21 01:06
> So why is it necessary (and a release blocker) to fix a bug in inspect._signature_fromstr, but a terrible idea to fix a bug in inspect.ismethod?

I was actually writing a comment, when I received this on my email ;) Let me explain my point of view.

> And the overwhelming majority voted +1 for "don't show self for bound methods".  So I'd like to change it for 3.4.

Sorry, I didn't know about it. I lost track of the relevant issue, and thought that we still need to keep the old behaviour in tact.

> inspect.ismethod() returns False for bound methods on builtins.

I'm +1 in principle, the only thing that bugs me, is that 'ismethod' is probably 100x more popular API then signature, and event getfullargspec. I'm just not sure what are the consequences.  Maybe, it's possible to fix pydoc without touching 'ismethod'?  My 2 cents.

Now, about the newly discovered bug in getfullargspec() [and _signature_fromstr]. I put a "release blocker" on that issue because it's a new change, that 'getfullargspec' now uses 'inspect.signature', and I don't think we should ship this new feature broken. But certainly, it's up to you to let this in 3.4.0.
msg211782 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-21 01:17
Okay, that's a fair point.  I checked, and the documentation specifically says that ismethod only returns true on bound methods implemented in Python.  I think that's a bad API, it should be agnostic about the implementation language.  But I'll remove the change to ismethod and rework the patch to pydoc around it.
msg211784 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-02-21 01:25
> I think that's a bad API, it should be agnostic about the implementation language.

Agree.

Re #20711: Please take a look at the patch I wrote. It's your code I modified, you know it and the __text_signature__ quirks better than anyone.
msg211785 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-21 01:28
Here's a revised patch that doesn't modify inspect.
msg211786 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-21 01:51
A slight tweak to the patch.  Previously I was just using truth testing on the value I got from "__self__", but that's wrong if the object is considered false (e.g. ''.zfill).  (Yury got this right in #20711, and I copied from him!)
msg211794 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-21 03:11
An even slighter tweak to the patch, just using De Morgan's law to make the code easier to read.
msg211811 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-21 06:52
Updated with tests.  And I built without docstrings and it still passed.  And if I revert the change to Lib/pydoc.py the tests fail.
msg211813 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-21 07:13
I went ahead and added tests for os.stat, and the unbound versions of the two class methods.  Total of five new tests now.
msg211814 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-21 07:20
You're right, that was the exact same test ;-)

So what I did was: move the other tests down into that test class, as that's a better fit, and keep the new version of the test.
msg211815 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-02-21 07:31
Looks good to me. Those unit tests give some confidence ;)
msg211816 - (view) Author: Roundup Robot (python-dev) Date: 2014-02-21 07:35
New changeset 5e73bb72662e by Larry Hastings in branch 'default':
Issue #20710: The pydoc summary line no longer displays the "self" parameter
http://hg.python.org/cpython/rev/5e73bb72662e
msg211818 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-02-21 07:43
With this patch 'help' now correctly renders classmethods, yay
msg213798 - (view) Author: Roundup Robot (python-dev) Date: 2014-03-17 06:30
New changeset b2ee3fe195e2 by Larry Hastings in branch '3.4':
Issue #20710: The pydoc summary line no longer displays the "self" parameter
http://hg.python.org/cpython/rev/b2ee3fe195e2
History
Date User Action Args
2014-03-17 06:30:44python-devsetmessages: + msg213798
2014-02-21 07:43:18yselivanovsetmessages: + msg211818
2014-02-21 07:37:16larrysetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2014-02-21 07:35:01python-devsetnosy: + python-dev
messages: + msg211816
2014-02-21 07:31:55yselivanovsetmessages: + msg211815
2014-02-21 07:20:16larrysetfiles: + larry.fix.help.on.bound.methods.8.diff

messages: + msg211814
2014-02-21 07:13:37larrysetfiles: + larry.fix.help.on.bound.methods.7.diff

messages: + msg211813
2014-02-21 06:52:29larrysetfiles: + larry.fix.help.on.bound.methods.6.diff

messages: + msg211811
2014-02-21 03:11:52larrysetfiles: + larry.fix.help.on.bound.methods.5.diff

messages: + msg211794
2014-02-21 01:51:42larrysetfiles: + larry.fix.help.on.bound.methods.4.diff

messages: + msg211786
2014-02-21 01:28:32larrysetfiles: + larry.fix.help.on.bound.methods.3.diff
2014-02-21 01:28:24larrysetmessages: + msg211785
2014-02-21 01:25:19yselivanovsetmessages: + msg211784
2014-02-21 01:17:02larrysetmessages: + msg211782
title: Make inspect and pydoc consistent about bound methods -> Make pydoc consistent about bound methods
2014-02-21 01:06:47yselivanovsetmessages: + msg211779
2014-02-21 00:58:46larrysetmessages: + msg211778
2014-02-21 00:54:47yselivanovsetmessages: + msg211777
2014-02-21 00:41:10yselivanovsetmessages: + msg211775
2014-02-21 00:38:13larrysetmessages: + msg211774
2014-02-21 00:37:15larrysetmessages: + msg211773
2014-02-21 00:36:54yselivanovsetmessages: + msg211772
2014-02-21 00:29:59larrysetfiles: + larry.fix.help.on.bound.methods.2.diff

messages: + msg211771
2014-02-21 00:25:41yselivanovsetmessages: + msg211770
2014-02-21 00:23:29yselivanovsetmessages: + msg211769
2014-02-21 00:19:41larrysettype: behavior
stage: patch review
2014-02-21 00:19:31larrycreate