[GDNative] Add missing join method for PoolStringArray class
#55826
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
akien-mga
reviewed
Dec 12, 2021
modules/gdnative/gdnative_api.json
Outdated
Comment on lines
2100
to
2107
| { | ||
| "name": "godot_pool_string_array_join", | ||
| "return_type": "godot_string", | ||
| "arguments": [ | ||
| ["godot_pool_string_array *", "p_self"], | ||
| ["const godot_string *", "p_delimiter"] | ||
| ] | ||
| }, |
There was a problem hiding this comment.
This would need to be defined in a new CORE API 1.3 here: https://github.com/godotengine/godot/blob/5dafc2cab1cad3d4022761619166e3502f63005e/modules/gdnative/gdnative_api.json#L20
There was a problem hiding this comment.
Thank you! Could you also check this PR: #55650 ? I guess I need to do the same there?..
There was a problem hiding this comment.
Done. I've updated gdnative_api.json. I've added new CORE API 1.3 and put newly added method within it.
Please, review 🙂
akien-mga
force-pushed
the
3.x
branch
2 times, most recently
from
71cb8d3 to
c58391c
Compare
kdiduk
force-pushed
the
gdnative-poolstringarray-join
branch
from
5dafc2c to
b67a36a
Compare
…ay class The class PoolStringArray in GDScript has `join` method, and it even has documentation. However, the corresponding definition of this method in GDNative headers were missing. In this commit, the missing GDNative definition of `join` method has been added. A new CORE API version 1.3 has been added with the new metod `join`.
kdiduk
force-pushed
the
gdnative-poolstringarray-join
branch
from
b67a36a to
bf60d65
Compare
|
Looks good to me overall. I'm not super familiar with GDNative so I don't know all the consequences of opening up a new CORE API. Would be good if @BastiaanOlij or other folks familiar with GDNative could have a look. If we do decide to add this as CORE API 1.3, we should merge this soon before the 3.5-stable release, so that this 1.3 API can match the 3.5 Godot release. There might also be more core type methods which have been added to the engine but not exposed to GDNative. If we do open up CORE API 1.3 for 3.5, we might as well want to make sure that we include those other missing methods. One such example is |
|
So I finally went through the list of all Variant methods in Here's the full list of methods in All Variant methods (minus constructors and operators)I then grepped all methods from that list in So there's quite a few! There's more than I expected and while that would be nice to have, I think implementing all those in a few days for Core API 1.3 in time for 3.5-stable would be a bit difficult. And since this is solved in Godot 4.0 (no longer needs manual reimplementation of those methods with GDExtension), I think we can keep it on an on-demand basis. So I think we can merge this PR and just have this tiny addition in Core API 1.3 for 3.5. And we can open Core API 1.4 for 3.6 if there's demand for more of these methods. Directly relevant to this PR though, implementing |
I can add Also, there is related PR #55650, but it has some merge conflicts that I have to resolve. If this PR is merged, I can put the methods in #55650 in the Core API 1.3 as well. So, if we merge this PR soon, we can still add some other methods in 1.3 😄 |
|
Alright, let's do that :) |
akien-mga
approved these changes
Jul 2, 2022
|
Thanks! |
This was referenced Jul 2, 2022
This was referenced Jul 16, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The class PoolStringArray in GDScript has
joinmethod, and it even has documentation.However, the corresponding definition of this method in GDNative headers were missing.
In this PR, the missing GDNative definition of
joinmethod has been added to the corresponding source files.This PR fixes #53116