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: Add CONFUSABLE_CAPTURE_REASSIGNMENT warning #93691

Merged

Conversation

dalexeev
Copy link
Member

@dalexeev dalexeev commented Jun 28, 2024

This is not a bug fix, but I think it would be nice to add the warning in 4.3, since this behavior is quite confusing while the change is simple and low risk.

@dalexeev dalexeev added this to the 4.3 milestone Jun 28, 2024
@dalexeev dalexeev requested review from a team as code owners June 28, 2024 06:43
@dalexeev dalexeev force-pushed the gds-confusable-capture-reassignment branch from 0feb552 to 7b0ff30 Compare June 28, 2024 06:53
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Both the name and documentation description of this warning do not mean much to the user that is going to stumble upon this warning while writing a lambda function.

@dalexeev
Copy link
Member Author

@Mickeon What exactly causes you concern? Is the problem in the wording or terminology? Can you suggest a better option?

@Mickeon
Copy link
Contributor

Mickeon commented Jun 28, 2024

The warning's name sounds correct, but it's a bit hard to understand. "Confusable", but why? It's somewhat broad. An assignment can be "confusing" for a few other reasons too.
Nowhere in the description it is stated that this is explicitly for lambdas, too. You may be able to infer it because you know how "capturing" works, but to whoever this warning may be beneficial to, that's not guaranteed.

I have no other better suggestions at the moment. I just know that because this is a very specific warning, it needs a very specific name (potentially just mentioning lambdas/callables directly?).

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Makes sense to me.

I agree that it could use mentioning lambdas at least in the description, and maybe in the warning's name (but that will start being long).
People familiar with lambdas will understand what capture refers to, but people not familiar with lambdas will also be going through the list of warnings to see what's useful to them, so it would help to scope it better to the relevant concept.

@dalexeev
Copy link
Member Author

Nowhere in the description it is stated that this is explicitly for lambdas, too.

You are right, I forgot to add this to the Project Settings description, although it is present in the warning message: Reassigning lambda capture does not modify the outer local variable "%s".

I agree that it could use mentioning lambdas at least in the description, and maybe in the warning's name (but that will start being long).

I thought about CONFUSABLE_LAMBDA_CAPTURE_REASSIGNMENT, but then that would be the longest warning code.

@dalexeev dalexeev force-pushed the gds-confusable-capture-reassignment branch from 7b0ff30 to 68898db Compare June 28, 2024 08:12
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Begrudgingly we cannot have warning names that span the width of your monitor.

@dalexeev
Copy link
Member Author

Begrudgingly we cannot have warning names that span the width of your monitor.

@akien-mga akien-mga merged commit 90bd2c2 into godotengine:master Jun 28, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

Closure fails to update primitive parameter
4 participants