Skip to content

Fix lookup symbol for super() #108306

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix lookup symbol for super() #108306

wants to merge 1 commit into from
+30 −1

Conversation

Chaosus
Copy link
Member

@Chaosus Chaosus commented Jul 5, 2025 •
edited
Loading

@Chaosus Chaosus requested a review from a team as a code owner July 5, 2025 09:47
@AThousandShips AThousandShips added this to the 4.x milestone Jul 5, 2025
@Chaosus Chaosus force-pushed the gds_fix_super_completion branch 2 times, most recently from def74d7 to 61e6789 Compare July 5, 2025 10:24
HolonProduction
HolonProduction reviewed Jul 7, 2025
View reviewed changes
@Chaosus Chaosus force-pushed the gds_fix_super_completion branch 5 times, most recently from dddda6b to 1c59c85 Compare July 7, 2025 14:23
@Chaosus Chaosus changed the title Fix completion for super() Fix symbol lookup for super() Jul 7, 2025
@Chaosus Chaosus force-pushed the gds_fix_super_completion branch from 1c59c85 to 67aef7a Compare July 7, 2025 14:25
@Chaosus Chaosus changed the title Fix symbol lookup for super() Fix lookup symbol for super() Jul 7, 2025
@Chaosus Chaosus requested a review from HolonProduction July 7, 2025 14:53
HolonProduction
HolonProduction reviewed Jul 8, 2025
View reviewed changes
Copy link
Member

@HolonProduction HolonProduction left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright you have convinced me. Let's add a new context type for super. The fact that we need different lookup depending on whether it is a call makes this pretty much impossible to solve without parser changes.

@HolonProduction
Copy link
Member

Should have some test. We don't have a specific test suite for lookup AFAIK, but the LSP test suite uses lookup under the hood. So we should add a supper call in one of those test cases.

@Chaosus Chaosus force-pushed the gds_fix_super_completion branch 2 times, most recently from eda6e35 to 9ce6f4e Compare July 8, 2025 13:20
@Chaosus Chaosus requested a review from a team as a code owner July 8, 2025 13:20
@Chaosus Chaosus force-pushed the gds_fix_super_completion branch from 9ce6f4e to c232b7c Compare July 8, 2025 13:21
@Chaosus
Copy link
Member Author

Chaosus commented Jul 8, 2025

Should have some test. We don't have a specific test suite for lookup AFAIK, but the LSP test suite uses lookup under the hood. So we should add a supper call in one of those test cases.

Done.. I think. Check it up.

HolonProduction
HolonProduction approved these changes Jul 8, 2025
View reviewed changes
Copy link
Member

@HolonProduction HolonProduction left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Calinou
Calinou approved these changes Jul 8, 2025
View reviewed changes
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code and tests look good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Calinou Calinou Calinou approved these changes

@HolonProduction HolonProduction HolonProduction approved these changes

Assignees
No one assigned
Labels
enhancement topic:editor topic:gdscript
Projects
None yet
Milestone
4.x
Development

Successfully merging this pull request may close these issues.

super() lacks lookup symbol in GDScript editor
4 participants
@Chaosus @HolonProduction @Calinou @AThousandShips