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

Improve error message when trying to instantiate a script with _init(...) #86635

Conversation

HolonProduction
Copy link
Member

@HolonProduction HolonProduction requested a review from a team as a code owner December 30, 2023 11:36
@AThousandShips
Copy link
Member

@Mickeon
Copy link
Contributor

Mickeon commented Dec 30, 2023

Is it possible, and could it be nice to explicitly state how many parameters are required?

@HolonProduction
Copy link
Member Author

HolonProduction commented Dec 30, 2023

Is possible. But doesn't help in the context of the proposal since no parameters can be passed. But yeah the error contains the expected amount of args so adding it to the message is possible.

Priority should however be to determine whether #76632 is the better solution or if they could coexist.

@AThousandShips
Copy link
Member

AThousandShips commented Dec 30, 2023

I'd say that #76632 is more comprehensive as it also adds error information (ignore the accidental inclusion of unrelated code by the OP of that PR, hopefully that'll get fixer or a new branch made)

Having explicit information about this specific case might be good, but it could also be confusing when not creating an instance like this, as the same error is thrown then, having an error message with information is what matters here, having it say "too few arguments" would be enough on its own

The current issue is the complete lack of information about what went wrong

It also covers one case this does not

Also this solution prints this message when creating an instance with too few arguments yet not none, as it is called in other situations as well, not just resource initialization, so I think the other PR is better for this

@HolonProduction
Copy link
Member Author

Adding the call error is definitely right. (I could probably include the changes into this PR to resolve the rebase mess up). I am just curious whether it is helpful in the context of godotengine/godot-proposals#8488. Or if we need to extend the error message to be helpful in that context. I'll have a look at the specific message.

@HolonProduction
Copy link
Member Author

Closing in favor of #76632

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.

Improve error message when Godot is unable to construct GDScriptInstance
3 participants