bpo-19903: IDLE: Calltips changed to use inspect.signature #2822

mlouielu · 2017-07-23T08:53:49Z

This commit change the get_argspec from using inspect.getfullargspec
to inspect.signature. It will improve the tip message for use.

Also, if object is not callable, now will return this message for
user, not blank tips. If the methods has an invalid method signature,
it will also return message to user.

Re-open from #1382

https://bugs.python.org/issue19903

mention-bot · 2017-07-23T08:53:51Z

@mlouielu, thanks for your PR! By analyzing the history of the files in this pull request, we identified @terryjreedy, @kbkaiser and @serhiy-storchaka to be potential reviewers.

terryjreedy · 2017-07-24T23:41:14Z

Louie, I cannot add commits to this issue, and a few others of yours and others. Please follow the instructions at https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/#enabling-repository-maintainer-permissions-on-existing-pull-requests to make sure "Allow edits from maintainers." is checked for this and other IDLE issues. Please let me know what you found. If that is already checked, then I have some other problem with github.

mlouielu · 2017-07-25T01:15:48Z

@terryjreedy The check is on now. Hmmm, I thought that is default to check-up? FWIR this has been discussed on python-committers before?

terryjreedy · 2017-08-02T21:46:04Z

There seems to have been a problem with a robot unchecking the edit switch under some circumstances. Since patched.

terryjreedy · 2017-08-02T23:00:02Z

I want to make a few minor changes, do some live tests, and merge. But when I tried to make a local PR, git trapped my clone in a loop. ~~See core-mentorship.~~ Fixed

terryjreedy

I wrote minor changes I planned to write, to make code slightly clearer. The should not affect tests.

PR also needs a blurb added. The title line might be enough. In any case, web edit works well for blurbs.

terryjreedy · 2017-08-02T23:02:24Z

Lib/idlelib/calltips.py

@@ -139,17 +141,31 @@ def get_argspec(ob):
        ob_call = ob.__call__
    except BaseException:
        return argspec


/argspec = ""/argspec = default = ""/ above
/argspec/default/ here
-- done

terryjreedy · 2017-08-02T23:02:49Z

Lib/idlelib/calltips.py

-    if isinstance(ob, type):
-        fob = ob.__init__
-    elif isinstance(ob_call, types.MethodType):
+    if isinstance(ob_call, types.MethodType):
        fob = ob_call
    else:
        fob = ob


Replace with conditional expression.
-- done

terryjreedy · 2017-08-02T23:04:43Z

Lib/idlelib/calltips.py

+            pass
+        else:
+            """Callable is not supported by signature"""
+            pass


Replace body with conditional expression:
return _invalid_method if str(err).startswith(_invalid_method) else default

If we do this, it will break the unittest, for e.g. range will not able to return argspec.

Right, in these cases, we still look for docstring.

This commit change the get_argspec from using inspect.getfullargspec to inspect.signature. It will improve the tip message for use. Also, if object is not callable, now will return this message for user, not blank tips. If the methods has an invalid method signature, it will also return message to user.

terryjreedy

I am doing some manual checking while waiting for CI, but expect to merge.

mlouielu · 2017-08-04T02:16:48Z

@terryjreedy Can this be backported to 3.5 and 3.6?

terryjreedy · 2017-08-04T02:46:49Z

Funny you should ask. When 3.6.0b1 came out, I stopped backporting to anything before 3.6: too much work and too much danger of regressions. In testing this, I noticed that ')' no longer closes calltips -- a bad regression since 3.6.2. Since repository 3.6.2+ has the same problem, it is definitely not this patch. I suspect bpo-30723. Of course, there is no test of such behaviors.

That aside, this patch would have less effect in 3.5 (pre-Arg Clinic, I believe), and the
last 3.5 maintenance release candidate is already out, so it is effectively in security-fix only mode.

too dangerousCurrently, everything is backported to 3.6, and only 3.6. The last 3.5 maintenance release candidate was issued a week ago, so it is effectively in security-only mode.

terryjreedy · 2017-08-04T02:51:29Z

Lib/idlelib/calltips.py

@@ -123,6 +123,8 @@ def get_entity(expression):
 _INDENT = ' '*4  # for wrapped signatures
 _first_param = re.compile(r'(?<=\()\w*\,?\s*')
 _default_callable_argspec = "See source or doc"
+_invalid_method = "invalid method signature"
+_argument_positional = "('/' marks preceding arguments as positional-only)"


Upon seeing a real example, I don't like the clash between () as call operator and comment marker in tips such as

(object, /)
('/' marks preceding arguments as positional-only)
Append object to the end of the list.

I prefer either [] or # as the comment marker. What think you?

(object, /)
['/' marks preceding arguments as positional-only]
Append object to the end of the list.

(object, /)
# '/' marks preceding arguments as positional-only
Append object to the end of the list.

I'll prefer ['/' marks preceding arguments as positional-only] style, it has more conspicuous when the text was end at the box margin.

Also, I think the word is OK, using google to search will got this stackoverflow post at the top, it should not be a problem for newbie.

By 'word', I presume you meant 'positional-only'. Nice check. We'll go with your preference.

terryjreedy · 2017-08-04T02:55:58Z

Lib/idlelib/calltips.py

+
+    if '/' in argspec:
+        """Using AC's positional argument should add the explain"""
+        argspec = '\n'.join([argspec, _argument_positional])


argspec += '\n' + _argument_positional

I'll fix _argument_positional to "\n['/' marks preceding arguments as positional-only]\n"

Even better.

terryjreedy · 2017-08-04T03:01:17Z

Lib/idlelib/idle_test/test_calltips.py

+non-overlapping occurrences of the pattern in string by the
+replacement repl.  repl can be either a string or a callable;
+if a string, backslash escapes in it are processed.  If it is
+a callable, it's passed the match object and must return''')


I would prefer that this be truncated also. We allowed up to 5 docstring lines for builtins to get the signature (bytes needs 5), but if we already have signature, only 1 should be allowed, as below. However, after checking the code and the docstring, I decided that the source of the problem is that the re.sub docstring is defective in not having an overly long summary sentence and then not having a blank line after the first '.', as it should. We could check for '.', but not worth the effort now. Instead I would like to add a comment so it is clear that this case was thought about. This is a good additional test case.

# Comment runs on because of long first sentence and missing blank thereafter. Display as above.

mlouielu · 2017-08-08T04:13:44Z

This PR should be block until the regression of not closing tipbox been fixed.

I don't think the problem came from #2306, I test it on mac first git pr 2822, then git revert fae2c3538e, the problem still exists.

mlouielu · 2017-08-08T05:19:11Z

Block fixed by 8922587

terryjreedy

Requested changes made, some with improvements.

bedevere-bot · 2017-08-10T01:18:32Z

GH-3053 is a backport of this pull request to the 3.6 branch.

…hon#2822)

…honGH-2822) Idlelib.calltips.get_argspec now uses inspect.signature instead of inspect.getfullargspec, like help() does. This improves the signature in the call tip in a few different cases, including builtins converted to provide a signature. A message is added if the object is not callable, has an invalid signature, or if it has positional-only parameters. Patch by Louie Lu.. (cherry picked from commit 3b0f620)

…2822) (#3053) Idlelib.calltips.get_argspec now uses inspect.signature instead of inspect.getfullargspec, like help() does. This improves the signature in the call tip in a few different cases, including builtins converted to provide a signature. A message is added if the object is not callable, has an invalid signature, or if it has positional-only parameters. Patch by Louie Lu.. (cherry picked from commit 3b0f620)

the-knights-who-say-ni added the CLA signed label Jul 23, 2017

terryjreedy mentioned this pull request Jul 24, 2017

bpo-19903: IDLE: Change to inspect.signature for calltips #1382

Closed

terryjreedy self-requested a review August 2, 2017 21:46

terryjreedy self-assigned this Aug 2, 2017

terryjreedy requested changes Aug 2, 2017

View reviewed changes

mlouielu force-pushed the bpo-19903 branch from 2f354d3 to 3969228 Compare August 3, 2017 06:05

mlouielu and others added 6 commits August 3, 2017 14:06

Merge branch 'master' into bpo-19903

6c8e67b

Add blurb

b4bb5dc

Addressed Terry comments

7ba5202

Cleanup

a9de237

Revert partial code

39907c7

Update 2017-08-03-14-08-42.bpo-19903.sqE1FS.rst

c495b61

terryjreedy approved these changes Aug 4, 2017

View reviewed changes

terryjreedy added the needs backport to 3.6 label Aug 4, 2017

terryjreedy requested changes Aug 4, 2017

View reviewed changes

terryjreedy reviewed Aug 4, 2017

View reviewed changes

Update 2017-08-03-14-08-42.bpo-19903.sqE1FS.rst

68fccf2

terryjreedy requested changes Aug 4, 2017

View reviewed changes

Fix _argument_positional words

55032cf

Merge branch 'master' into bpo-19903

134cca4

terryjreedy approved these changes Aug 10, 2017

View reviewed changes

terryjreedy merged commit 3b0f620 into python:master Aug 10, 2017

bedevere-bot removed the needs backport to 3.6 label Aug 10, 2017

terryjreedy pushed a commit to terryjreedy/cpython that referenced this pull request Aug 10, 2017

[3.6] bpo-19903: IDLE: Calltips changed to use inspect.signature (pyt…

f937f33

…hon#2822)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-19903: IDLE: Calltips changed to use inspect.signature #2822

bpo-19903: IDLE: Calltips changed to use inspect.signature #2822

mlouielu commented Jul 23, 2017 •

edited by bedevere-bot

Loading

mention-bot commented Jul 23, 2017

terryjreedy commented Jul 24, 2017

mlouielu commented Jul 25, 2017

terryjreedy commented Aug 2, 2017

terryjreedy commented Aug 2, 2017 •

edited

Loading

terryjreedy left a comment

terryjreedy Aug 2, 2017 •

edited

Loading

terryjreedy Aug 2, 2017 •

edited

Loading

terryjreedy Aug 2, 2017

mlouielu Aug 3, 2017

terryjreedy Aug 4, 2017

terryjreedy left a comment

mlouielu commented Aug 4, 2017

terryjreedy commented Aug 4, 2017

terryjreedy Aug 4, 2017 •

edited

Loading

mlouielu Aug 8, 2017

terryjreedy Aug 10, 2017

terryjreedy Aug 4, 2017

mlouielu Aug 8, 2017

terryjreedy Aug 10, 2017

terryjreedy Aug 4, 2017

mlouielu commented Aug 8, 2017

mlouielu commented Aug 8, 2017

terryjreedy left a comment

bedevere-bot commented Aug 10, 2017

bpo-19903: IDLE: Calltips changed to use inspect.signature #2822

bpo-19903: IDLE: Calltips changed to use inspect.signature #2822

Conversation

mlouielu commented Jul 23, 2017 • edited by bedevere-bot Loading

mention-bot commented Jul 23, 2017

terryjreedy commented Jul 24, 2017

mlouielu commented Jul 25, 2017

terryjreedy commented Aug 2, 2017

terryjreedy commented Aug 2, 2017 • edited Loading

terryjreedy left a comment

Choose a reason for hiding this comment

terryjreedy Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

terryjreedy Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terryjreedy left a comment

Choose a reason for hiding this comment

mlouielu commented Aug 4, 2017

terryjreedy commented Aug 4, 2017

terryjreedy Aug 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlouielu commented Aug 8, 2017

mlouielu commented Aug 8, 2017

terryjreedy left a comment

Choose a reason for hiding this comment

bedevere-bot commented Aug 10, 2017

mlouielu commented Jul 23, 2017 •

edited by bedevere-bot

Loading

terryjreedy commented Aug 2, 2017 •

edited

Loading

terryjreedy Aug 2, 2017 •

edited

Loading

terryjreedy Aug 2, 2017 •

edited

Loading

terryjreedy Aug 4, 2017 •

edited

Loading