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

GDScript: Fix incorrect default values ​​in _make_arguments_hint() #94664

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

dalexeev
Copy link
Member

1. Fix concatenation of the function name with reduced value.

case GDScriptParser::Node::CALL: {
const GDScriptParser::CallNode *call = static_cast<const GDScriptParser::CallNode *>(par->initializer);
if (call->is_constant && call->reduced) {
def_val = call->function_name.operator String() + call->reduced_value.operator String();
} else {
def_val = call->function_name.operator String() + (call->arguments.is_empty() ? "()" : "(...)");
}

2. Use Variant::get_construct_string() instead of Variant::operator String() for consistency with native hints and GDScriptDocGen.

if (i - def_args >= 0) {
arghint += String(" = ") + p_info.default_arguments[i - def_args].get_construct_string();
}

default:
return p_variant.get_construct_string();

3. Fix GDScriptParser::Node::SUBSCRIPT case. For enum values ​​the logic has been corrected; for other types it has been discarded since the previous assumptions are incorrect.

@dalexeev dalexeev added this to the 4.3 milestone Jul 23, 2024
@dalexeev dalexeev requested a review from a team as a code owner July 23, 2024 16:51
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.

Have tested it locally and it indeed fixes the issue. Code makes sense as well.

The usage of get_constructor_string also fixes quotes around reduced string values not showing properly. Though I doubt there is an issue about it, since we only show the reduced value in the subscript case, speaking of which, maybe you could copy this is_constant && reduced condition to the IDENTIFIER case, to avoid a one line PR for that in the future.

Also I think it would be nice to have some unit tests for this, the completion test runner can check the argument hint. But given that there aren't any tests at all for argument hints, we could also just merge the fix as is and I'll work on a complete set of tests for argument hints.

@dalexeev
Copy link
Member Author

maybe you could copy this is_constant && reduced condition to the IDENTIFIER case

Actually, it will make readability worse. Instead of my_param = MY_CONST you will get my_param = 123. This is inconsistent with GDScriptDocGen, but I prefer not to change it within this PR.

is_constant && reduced

TODO: Change this to just is_constant.

Also I think it would be nice to have some unit tests for this, the completion test runner can check the argument hint. But given that there aren't any tests at all for argument hints, we could also just merge the fix as is and I'll work on a complete set of tests for argument hints.

Agree, let's do it in a separate PR.

@akien-mga akien-mga merged commit f361133 into godotengine:master Jul 24, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@dalexeev dalexeev deleted the gds-fix-make-arguments-hint branch July 24, 2024 08:59
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.

Code autocomplete text for default constructed Callable is incorrect (extra "null" string)
3 participants