Rename type_string to type_to_string

[sirjoehighton]

Renamed method type_string to type_to_string for better clarity and consistency with naming conventions in the Godot Engine. As well as altering the documentation to reflect this change in all supported languages.

[AThousandShips]

Thank you for your contribution

This can't be done without major compatibility breakage, there's no way to register compatibility methods for these methods, which makes this not possible to handle gracefully

Renaming this serves little purpose, and breaks all existing code using it

The way to solve this would be to deprecate the old method and add a new one, if that is done this can move forward to be discussed

[AThousandShips]

Have you discussed this need with anyone? Is this a problem people have raised on the forums or elsewhere? What is behind this change?

[sirjoehighton]

I understand the compatibility concerns. My goal was to improve clarity since type_string can be confusing alongside TYPE_STRING from Variant.Type.

[AThousandShips]

I'd say that confusion is minimal, and changing it would make it follow naming in general worse, considering error_string

[sirjoehighton]

I apologise if I make any mistakes as this is my first time contributing to the Engine and I'm not accustomed to this procedure yet.

[AThousandShips]

It's okay it's a learning experience! I think opening a proposal to gauge what people think would be a good step, if you're still interested in working on this

To make the necessary changes you would need to add #ifndef DISABLE_DEPRECATED around the original bind, and add a duplicate with the new name, and then duplicate the original documentation entry and add a deprecation message to it, like so: deprecated="Use [method type_to_string] instead."

[sirjoehighton]

If major compatibility breakage is the main issue this can wait til the next Major revision where backwards compability isnt much of an issue as far as im aware.

[AThousandShips]

That would be best I think, feel free to open a proposal to discuss things in the meantime

[sirjoehighton]

It's okay it's a learning experience! I think opening a proposal to gauge what people think would be a good step, if you're still interested in working on this

To make the necessary changes you would need to add #ifndef DISABLE_DEPRECATED around the original bind, and add a duplicate with the new name, and then duplicate the original documentation entry and add a deprecation message to it, like so: deprecated="Use [method type_to_string] instead."

Okay i might try this and see how it goes, thanks for the advice! It really helps.

[sirjoehighton]

Not exactly sure if i did this right but i tried atleast!

Resolved

[sirjoehighton]

Looks like all the checks have passed 👍🏼

[kleonc]

I think currently it's adding more bloat for a minimal (and debatable) improvement, hence I'm rather against such rename (in 4.x).

Renamed method type_string to type_to_string for better clarity and consistency with naming conventions in the Godot Engine.

Note there's also error_string method following the same naming convention.

[sirjoehighton]

I think currently it's adding more bloat for a minimal (and debatable) improvement, hence I'm rather against such rename (in 4.x).

Renamed method type_string to type_to_string for better clarity and consistency with naming conventions in the Godot Engine.

Note there's also error_string method following the same naming convention.

I appreciate the feedback, but the goal here is to improve clarity, not bloat the code. The change is extremely minimal and aimed at making the method name more intuitive. I havent mentioned anything about "error_string" so it’s not relevant to the discussion.

[sirjoehighton]

To add to this the type_string method would eventually be removed and replaced in 5.x, where backward compatibility won't be necessary and no additional features have been added so this wouldn't add bloat in the end.

[AThousandShips]

I appreciate the feedback, but the goal here is to improve clarity, not bloat the code.

The bloat here is the deprecation

I havent mentioned anything about "error_string" so it’s not relevant to the discussion.

I brought it up before and the PR itself talks about "consistency with naming conventions in the Godot Engine" but error_string is an example of how this isn't a change that adds consistency

[kleonc]

but the goal here is to improve clarity, not bloat the code

That's obvious / I wouldn't assume anyone's intention is to bloat the code. 🙃 But as a result of this PR for now (in 4.x) we would have two methods: type_string and type_to_string, instead of only one type_string. This is the bloat I meant (two entries in the docs etc.).

The change is extremely minimal and aimed at making the method name more intuitive.

The change being extremely minimal is the problematic part to me. If the name would be completely wrong then renaming/fixing it would make sense. But only slightly tweaking the name to prevent potential minimal confusion I'd say is not enough of a justification.

I havent mentioned anything about "error_string" so it’s not relevant to the discussion.

It's absolutely relevant. If talking about naming consistency then already existing names should be taken into account. And because error_string exists and this PR (in its current form) doesn't do anything about it, I'd say it makes things more inconsistent.

Currently:

• type_string
• error_string

With this PR (in its current form):

• type_string
• type_to_string
• error_string (but there's no error_to_string?! 🤔)

---

To clarify: I'm not suggesting to add error_string -> error_to_string rename to this PR as well. I simply think such renames are not worth the burden. For 5.0 I'd say sure, currently I'm against.
I was all for renaming things when going from 3.x to 4.x, but currently I don't think such minor renames are justified.
(Of course all above is just my opinion (so you might want to wait for some more feeback).)

[RadiantUwU]

Requires a squash before merging.

[akien-mga]

Requires a squash before merging.

To clarify the PR workflow - a squash before merging would indeed be needed, but this PR hasn't been approved for merging by maintainers yet.

So I wouldn't request more work from @sirjoehighton when there are high chances that this PR may be rejected - as there's no proposal/explicit demand to back this up and two maintainers (@AThousandShips and @kleonc) already expressed that this may not be a good idea to do this change now.

[akien-mga]

Now as a maintainer review, I confirm what AThousandShip and kleonc already said, I don't think this is a good change at this stage. It adds unnecessary duplicates in the API, which makes the API IMO more confusing that the issue that the issue that this aims to address.

I'm also not convinced the "problem" is really one. I do see at least one user was confused by the API (at least I assume so if you propose to change it), but I don't see evidence that it's a widespread problem.

I think such a change would only make sense to do for a hypothetical Godot 5.0, but it's not planned currently, so I would prefer to simply not consider it. Even for Godot 5.0, I'm not sure we should still do "arbitrary" renames to make things slightly clearer like this, which in the 3.x to 4.0 transition were done quite a lot and seemed to upset a number of users having to deal with conversions that weren't a clear win in their eyes.

Thanks for the contribution nonetheless!

[dalexeev]

Belatedly, I'd like to add that in the future we should consider adding the ability to get enumerator names for all enums (global, native, custom). Then we can actually deprecate type_string() and error_string() as special cases. See also:

• Built-in named enum consistency proposal godot-proposals#1426
• Expose engine enums as scoped enums (to allow getting them as strings etc.) godot-proposals#2854
• Enum naming consistency godot-proposals#5624

EDIT: These two functions do not return the enumerator name, but return the type name and a human-readable error string respectively. So we should not deprecate them.