Skip to content

Conversation

@geoffbacon
Copy link
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
Copy link
Collaborator

dbieber commented Sep 7, 2019

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