-
-
Couldn't load subscription status.
- Fork 689
Specifically handle each "meta" value, so new ones don't break code generation #1868
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
Conversation
9eb0ad8 to
a977357
Compare
extension_api.json for arguments and return values|
I've decided to go a different way instead, and specifically address all the "meta" values we are aware of, so that things won't break in the future when new ones are added (including the "required" one) This should make this safe to be merged even before PR godotengine/godot#86079 and cherry-pick it |
|
|
||
| def correct_type(type_name, meta=None, use_alias=True): | ||
| type_conversion = {"float": "double", "int": "int64_t", "Nil": "Variant"} | ||
| if meta is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if meta is not None, but we don't handle it here (unexpected value)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will ignore it. That's actually the whole point of this PR: if there's an unexpected value in "meta", then we'll ignore it and not potentially generate invalid code.
PR godotengine/godot#86079 adds "required" as a potential value for "meta", and without this PR, it'll generate invalid code (attempting to use "required" as a type name).
There was a problem hiding this 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.
|
Cherry-picked for 4.5 in PR #1870 |
|
Cherry-picked for 4.4 in PR #1871 |
Originally, this PR specifically ignored the "required" meta that is added in PR godotengine/godot#86079
However, I've decided to go a different way instead, and specifically address all the "meta" values we are aware of, so that things won't break in the future when new ones are added (including the "required" one)
This should make this safe to be merged even before that PR and cherry-pick it
Original Description
This fixes issues in our code generation when used with Godot PR godotengine/godot#86079
It basically just makes it ignore the "required" meta, which may be the right thing to do for godot-cpp, at least as far as handling APIs from Godot, although, this is something we should discuss.
But even if we do do that, we should still add
RequiredParam<T>andRequiredValue<T>in godot-cpp for the purpose of marking APIs created in GDExtensions as having required parameters and values, but that can be a separate PR (since this one will need to be cherry-picked to Godot 4.5, but the further changes shouldn't be)