-
Notifications
You must be signed in to change notification settings - Fork 210
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
Unify string parameters across the repo #1037
base: master
Are you sure you want to change the base?
Unify string parameters across the repo #1037
Conversation
The types of string parameters across the repo has been regularized according to the following rules that shall be followed from now on: - Methods during the init/registration process should take `&str` for things like identifiers. They may specify explicit lifetimes in their types if necessary, but *not* `'static`. - Manually written methods that abstract over, or mimic API methods should take `impl Into<GodotString>` or `impl Into<NodePath>` depending on the usage, to be consistent with their generated counterparts. Regarding the usage of `&str` in init instead of more specific conversions required by each method: The GDNative API isn't very consistent when it comes to the string types it wants during init. Sometimes, functions may want different types of strings even when they are concepturally similar, like for signal and method registration. This gets more complicated when we add our own library-side logic into the mix, like for the `InitHandle::add_class` family, where allocation is current inevitable even when they are given `&'static str`s. It still makes sense to avoid excessive allocation, but that should not get in the way of future development. `'static` in particular is extremely limiting, and there are very few cases of its usage throughout the library's history that we have not since regretted. It shouldn't be the norm but the rare exception. In any case, class registration is something that only run once in the lifecycle of a game, and for the amount of classes most people are using with `gdnative`, we can afford to be slightly inefficient with strings to make our APIs more consistent, less leaky, and easier to stablize and maintain.
92eaeed
to
6f58d75
Compare
That's something that has always bothered me, very nice to see that you came up with guidelines to unify string usage! 👍
A similar argument could maybe be brought for error types like While they do not occur only once like registration, they should ideally happen rarely (although one could maybe more dynamic imagine use cases where these are more frequent). Maybe it's not as pressing, since the |
Indeed. Thanks for pointing it out! I think it should indeed be considered as a separate case, and perhaps also dealt with within this PR. Some of the variants of There is a minor point that errors like I guess it's a fair starting point to just make them all |
The types of string parameters across the repo has been regularized according to the following rules that shall be followed from now on:
&str
for things like identifiers. They may specify explicit lifetimes in their types if necessary, but not'static
.impl Into<GodotString>
orimpl Into<NodePath>
depending on the usage, to be consistent with their generated counterparts.Regarding the usage of
&str
in init instead of more specific conversions required by each method:The GDNative API isn't very consistent when it comes to the string types it wants during init. Sometimes, functions may want different types of strings even when they are concepturally similar, like for signal and method registration. This gets more complicated when we add our own library-side logic into the mix, like for the
InitHandle::add_class
family, where allocation is current inevitable even when they are given&'static str
s.It still makes sense to avoid excessive allocation, but that should not get in the way of future development.
'static
in particular is extremely limiting, and there are very few cases of its usage throughout the library's history that we have not since regretted. It shouldn't be the norm but the rare exception.In any case, class registration is something that only run once in the lifecycle of a game, and for the amount of classes most people are using with
gdnative
, we can afford to be slightly inefficient with strings to make our APIs more consistent, less leaky, and easier to stablize and maintain.Close #996