Skip to content
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

Remove unneeded quotes from autocomplete % nodes #96666

Merged
merged 1 commit into from
Sep 11, 2024

Conversation

aXu-AP
Copy link
Contributor

@aXu-AP aXu-AP commented Sep 6, 2024

Removes unnecessary quotes for suggestions like $"%MyNode".

@HolonProduction
Copy link
Member

The change makes sense and works. Could you maybe add a test case for it?
You could put it in here: https://github.com/godotengine/godot/tree/master/modules/gdscript/tests/scripts/completion/get_node/

Removes unnecessary quotes for suggestions like $"%MyNode".
@aXu-AP aXu-AP force-pushed the unique-node-no-quotes branch from 5faed09 to 2525311 Compare September 7, 2024 14:49
@aXu-AP
Copy link
Contributor Author

aXu-AP commented Sep 7, 2024

I tried to add a test, but I'm not sure if I did it right and if the location and name makes sense. For some reason I can't run tests locally (I did build with scons dev-mode=yes, dunno why it doesn't work)...

@HolonProduction
Copy link
Member

The test looks good to me and passes in CI, so you did it right 🎉

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 (still needs an official review though)

@aXu-AP
Copy link
Contributor Author

aXu-AP commented Sep 7, 2024

Thanks for reviewing and also for your work with autocompletion, I think it's the feature that makes or breaks the code editor 👍

@aXu-AP
Copy link
Contributor Author

aXu-AP commented Sep 10, 2024

By the way, do we have existing test case for $ operator's autocompletion? For basic case ($ -> SomeNode) and for quotes ($ -> "Other Node")? I didn't find them... I could add them here if they don't exist yet.

@HolonProduction
Copy link
Member

I was looking for some when I tried to link you to a location to add them and didn't find something. That being said, I'm currently changing something about the test runner, tests will probably be compatible but I wouldn't invest time into it before the new behavior is in.

@aXu-AP
Copy link
Contributor Author

aXu-AP commented Sep 10, 2024

Gotchu. If you want, CC me once the changes are in, I can do it afterwards :)

@akien-mga akien-mga merged commit 1ff2186 into godotengine:master Sep 11, 2024
20 checks passed
@akien-mga
Copy link
Member

Thanks!

@aXu-AP aXu-AP deleted the unique-node-no-quotes branch September 11, 2024 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants