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

Fix ScriptLanguageExtension::_find_function argument names #86520

Conversation

touilleMan
Copy link
Member

ScriptLanguageExtension::_find_function documentation is currently wrong when compared to it GDScript implementation:

int GDScriptLanguage::find_function(const String &p_function, const String &p_code) const {

@touilleMan touilleMan requested a review from a team as a code owner December 26, 2023 11:29
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

This can't be changed like this, the binding is here:

GDVIRTUAL_BIND(_find_function, "class_name", "function_name");

Not in GDScript, which isn't a ScriptLanguageExtension but a ScriptLanguage in the first place

The discrepancy is an issue, but this will not be a valid fix as the documentation generation won't match

@AThousandShips
Copy link
Member

The problem here is that I don't know that this can be changed without breaking compatibility with extensions, I'm not sure if these bound arguments are compatibility relevant for other languages and binds

@AThousandShips AThousandShips requested a review from a team December 26, 2023 12:09
@touilleMan
Copy link
Member Author

This can't be changed like this, the binding is here:

You're right I should have modified this part too ! (I've updated my PR)

Not in GDScript, which isn't a ScriptLanguageExtension but a ScriptLanguage in the first place

True, I was pointing out GDScript given it is the standard ScriptLanguage, and ScriptLanguageExtension is just a wrapper over ScriptLanguage.

The keypoint is that ScriptLanguageExtension exposes a _find_function method that is used to implement ScriptLanguage::find_function:

EXBIND2RC(int, find_function, const String &, const String &)

So in the end, ScriptLanguageExtension::_find_function should have the same signature as ScriptLanguage::find_function.

The problem here is that I don't know that this can be changed without breaking compatibility with extensions

The fix is only on the name of the parameters, are you sure this breaks compatibility ?

@touilleMan touilleMan force-pushed the fix-ScriptLanguageExtension-_find_function-documentation branch from 516ee5d to 0124b51 Compare December 26, 2023 13:06
@touilleMan touilleMan requested a review from a team as a code owner December 26, 2023 13:06
@AThousandShips
Copy link
Member

The fix is only on the name of the parameters, are you sure this breaks compatibility ?

I'm not sure, that's why I said "I don't know if this can be done without breaking compatibility" 🙂

CC @dsnopek

Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

I don't know that this can be changed without breaking compatibility

Yeah, I think this is fine! It doesn't change how the arguments are used, only their names. From the perspective of GDExtension, arguments are only positional, the names aren't significant.

@Chaosus Chaosus added this to the 4.3 milestone Dec 28, 2023
@akien-mga akien-mga merged commit 3dae50a into godotengine:master Jan 2, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga changed the title Fix ScriptLanguageExtension::_find_function documentation Fix ScriptLanguageExtension::_find_function argument names Jan 11, 2024
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.

5 participants