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.

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

Fix for GDScriptHighlighter dictionaries as function arguments #82326

Merged
merged 1 commit into from
Oct 5, 2023
Merged

Fix for GDScriptHighlighter dictionaries as function arguments #82326

merged 1 commit into from
Oct 5, 2023

Conversation

adeneve
Copy link
Contributor

@adeneve adeneve commented Sep 25, 2023

Fixes #81362

Dictionaries passed as function arguments were having their values highlighted green due to being misinterpreted as function argument hints.

Pics below and after code change.
beforePic
afterPic

@adeneve adeneve requested a review from a team as a code owner September 25, 2023 19:29
MewPurPur
MewPurPur approved these changes Sep 25, 2023
View reviewed changes
Copy link
Contributor

@MewPurPur MewPurPur left a comment

Choose a reason for hiding this comment

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

Looks good, mostly style nitpicks.

modules/gdscript/editor/gdscript_highlighter.cpp Outdated Show resolved Hide resolved
modules/gdscript/editor/gdscript_highlighter.cpp Outdated Show resolved Hide resolved
modules/gdscript/editor/gdscript_highlighter.cpp Outdated Show resolved Hide resolved
@MewPurPur
Copy link
Contributor

MewPurPur commented Sep 25, 2023 •
edited
Loading

While at it, you can also fix the comments near your changed lines to use "Sentence case." And don't forget to commit amend.

@adeneve adeneve force-pushed the gdscript_dict_highlighter_fix branch from 3790e45 to a050b3b Compare September 26, 2023 00:34
@adeneve
Copy link
Contributor Author

adeneve commented Sep 26, 2023

Commit has been amended 👍. Ready for another review.

@adeneve adeneve force-pushed the gdscript_dict_highlighter_fix branch from a050b3b to a8221eb Compare September 26, 2023 02:29
@dalexeev dalexeev added this to the 4.2 milestone Sep 26, 2023
@adeneve adeneve force-pushed the gdscript_dict_highlighter_fix branch from a8221eb to 6fa0eb5 Compare September 26, 2023 14:49
@MewPurPur MewPurPur mentioned this pull request Sep 29, 2023
Merged
@YuriSizov YuriSizov requested a review from dalexeev September 29, 2023 14:42
dalexeev
dalexeev approved these changes Sep 30, 2023
View reviewed changes
Copy link
Member

@dalexeev dalexeev left a comment

Choose a reason for hiding this comment

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

Looks good to me. I've tested and it works with braces in string literals. Perhaps in some cases like multiline code this does not work, but this is not something that we could fix within a small PR.

Also I noticed a bug with parentheses:

bool in_function_args = false; should be replaced with int in_function_args = 0; like in this PR.

@akien-mga akien-mga changed the title Fix for gdscript_highlighter dictionaries as function arguments Fix for GDScriptHighlighter dictionaries as function arguments Sep 30, 2023
@akien-mga
Copy link
Member

bool in_function_args = false; should be replaced with int in_function_args = 0; like in this PR.

@adeneve Would you be able to update this PR with this additional fix? If so, please do it by amending the commit, so it stays a single commit for both related fixes.

@adeneve
Copy link
Contributor Author

adeneve commented Oct 3, 2023

Hi @akien-mga , yes i'll try to take a look some time later today.

@adeneve adeneve force-pushed the gdscript_dict_highlighter_fix branch from 6fa0eb5 to c8f7f37 Compare October 3, 2023 19:34
@adeneve
Copy link
Contributor Author

adeneve commented Oct 3, 2023

Ready for another review. Commit has been amended to address the parentheses bug.

bool in_function_args = false; should be replaced with int in_function_args = 0; like in this PR.

MewPurPur
MewPurPur approved these changes Oct 3, 2023
View reviewed changes
Copy link
Contributor

@MewPurPur MewPurPur left a comment

Choose a reason for hiding this comment

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

Code review LGTM

@adeneve adeneve force-pushed the gdscript_dict_highlighter_fix branch from c8f7f37 to 73c8a95 Compare October 4, 2023 19:03
@adeneve adeneve force-pushed the gdscript_dict_highlighter_fix branch from 73c8a95 to d8ad0ca Compare October 5, 2023 19:37
@adeneve
Fix for gdscript_highlighter dictionaries as arguments
978fcaf 
Fix for gdscript_highlighter. When passing a dictionary
as a function argument, the dictionary values were being
highlighted green as if they were types.
@adeneve adeneve force-pushed the gdscript_dict_highlighter_fix branch from d8ad0ca to 978fcaf Compare October 5, 2023 19:38
@akien-mga akien-mga merged commit 1edf0f3 into godotengine:master Oct 5, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@dalexeev dalexeev mentioned this pull request Dec 14, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dalexeev dalexeev dalexeev approved these changes

@Scoppio Scoppio Scoppio left review comments

@MewPurPur MewPurPur MewPurPur approved these changes

Assignees
No one assigned
Labels
bug topic:editor topic:gdscript
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.

Incorrect GDScript highlighting of variables passed as values in dictionary function arguments.
5 participants
@adeneve @MewPurPur @akien-mga @Scoppio @dalexeev