Skip to content

Fix advanced class completion #932

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 21, 2021
Merged

Fix advanced class completion #932

merged 1 commit into from
Oct 21, 2021

Conversation

gpotter2
Copy link
Contributor

@gpotter2 gpotter2 commented Oct 21, 2021

This PR removes a check that always tried to autocomplete a class using its __init__ function.

In fact, the inspect module (used afterwards in

signature = inspect.signature(f)
) does a much better job at finding the arguments that are actually typing a class, which aren't always the ones in __init__. Since at least Python 3.6, it handles __call__, __new__ and __signature__ in addition to __init__, so restricting the signatures to __init__ is actually a downgrade.
See https://github.com/python/cpython/blob/2c56c97f015a7ea81719615ddcf3c745fba5b4f3/Lib/inspect.py#L2284

I'm raising this issue because our CLI requires __signature__ to work, which is the case if you let inspect.signature do its job by default but not if you manually select cls.__init__ 🙂

Note: I also doubt that the __new__ check below is useful on modern Python versions, since __new__ is also checked by inspect.signature (therefore by inspection.getfuncprops), but I didn't remove it in this PR.

@sebastinas
Copy link
Contributor

Could this change be accompanied with a test case to avoid regressions in the future?

@gpotter2
Copy link
Contributor Author

I've added a test.

@gpotter2
Copy link
Contributor Author

Just fixed the black issue.

@sebastinas sebastinas merged commit 126b059 into bpython:main Oct 21, 2021
@thomasballinger
Copy link
Member

Thanks for bringing bpython completion into the (5 years ago?) future!

@gpotter2 gpotter2 deleted the class-cmpl branch October 21, 2021 17:28
@sebastinas sebastinas added this to the release-0.22 milestone Oct 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants