Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Exposes builtin functions from the standard library#193

Merged
dbieber merged 3 commits into
google:mastergoogle/python-fire:masterfrom
geoffbacon:mastergeoffbacon/python-fire:masterCopy head branch name to clipboard
Sep 8, 2019
Merged

Exposes builtin functions from the standard library#193
dbieber merged 3 commits into
google:mastergoogle/python-fire:masterfrom
geoffbacon:mastergeoffbacon/python-fire:masterCopy head branch name to clipboard

Conversation

@geoffbacon

Copy link
Copy Markdown
Contributor

This PR exposes builtin functions from the standard library to fire and fixes #187.

Previously, certain builtin functions from the standard library (e.g. math.sin) would raise a TypeError complaining about getting one less required argument than they need. The original diagnosis was that these functions were written in C and did not expose their signature toinspect.getfullargspec or inspect.signature. However math.sin does expose its signature to these inspect functions:

>>> inspect.getfullargspec(math.sin)
FullArgSpec(args=['x'], varargs=None, varkw=None, defaults=None, kwonlyargs=
[], kwonlydefaults=None, annotations={})

The problem was that fire.inspectutils._GetArgSpecInfo was incorrectly skipping the first argument of these builtins under the assumption that a non-null __self__ attribute of a builtin always means that the function is a method and its first argument is self. However, for builtin functions in standard libraries, the __self__ attribute is the module it came from. The proposed fix is to only skip the first argument of builtins if the __self__ attribute isn't a module.

@googlebot googlebot added the cla: yes Author has signed CLA label Sep 6, 2019
@dbieber

dbieber commented Sep 7, 2019

Copy link
Copy Markdown
Collaborator

Thanks for the fix! LGTM.
Travis is complaining about the indentation; we use 2 spaces.
Will squash and merge soon - prefer if you fix the indentation but if not I will fix in a follow up commit.

🎉

@dbieber dbieber merged commit 8d372e2 into google:master Sep 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Author has signed CLA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cannot expose built-in functions from the standard library

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.