gdnative icon indicating copy to clipboard operation
gdnative copied to clipboard

Naming inconsistencies

Open Bromeon opened this issue 3 years ago • 4 comments

This issue collects godot-rust symbols, whose current names could be made more expressive in a future version. This includes anything: types, functions, macros, variables. It excludes generated names from the GDNative bindings, as they are not chosen by us*.

All of these are up to discussion, of course. Migration could happen step-wise: v0.10 deprecates the names and adds type aliases for the new names (or for old ones), and v0.11 removes old names.

  • [x] RefInstance -> TInstance Analogous to Ref and TRef, the latter is the temporary, lifetime-bound variant of the former. RefInstance suggests that this is a mix between Ref and Instance.

  • [x] TypedArray -> PoolArray Would immediately reflect that it's the Rust counterpart of GDScript's Pool*Arrays. We need to decide if it makes sense to keep Int32Array and Float32Array around.

  • [ ] VariantArray -> Array (?) This one is a bit less clear: while it would match GDScript's type Array, the name "Array" is very general and doesn't clearly describe the purpose. On the other hand, the same problem occurs with Dictionary. However, "dictionary" is quite specific to GDScript terminology; in Rust such types are typically called "maps".

  • [ ] Conversions between Ref, TRef, Instance, Script etc. Some terms are used interchangeably (base/owner), while some are not related to the conversion involved (claim). This is OK to some extent, but we need to be careful that there are not too many names that need to be "learnt by heart", as that makes APIs less accessible.

  • [x] Type states; e.g. Access -> Ownership See this comment.

Related, but planned for v0.10: #712


* Note regarding GDNative types

A lot of them have multiple capital letters, e.g. ARVRCamera, AudioStreamOGGVorbis, PCKPacker, UPNP, WebRTCPeerConnection and more. Would likely cause more confusion than benefit to "correct" them, add lots of manual special cases, and sometimes become obsolete in Godot 4. What needs to be checked though is why this was done for some types like G6dofJointAxisParam. Godot seems to use G6DOFJointAxisParam, and GDNative apparently too. But where does the de-capitalization in godot-rust originate?

Bromeon avatar Aug 27 '21 16:08 Bromeon

@Bromeon I like the idea of simplifying these.

General thoughts

  • TInstance is a lot easier to immeditately comprehend than the current RefInstance
  • Making the array types (across the board) match their GDScript counterparts is a good idea. Sometimes it can be really tricky to figure how which one you actually need.
  • I think that the only exception to the above is VariantArray. Since it must hold only Variant typed objects, it could be worth keeping that clearly stated in the name. Fortunately if we make the change to Array since Rust arrays are declared [a, b, c; 3] there's little risk of confusion or name collision.
  • Access should probably be Ownership since that's the real semantics being used. (Unless Ownership is a reserved keyword or type or something).

Regarding the conversions between types, is the goal to create some extension methods to help with the conversions?

jacobsky avatar Sep 27 '21 15:09 jacobsky

Thanks!

I think that the only exception to the above is VariantArray. Since it must hold only Variant typed objects, it could be worth keeping that clearly stated in the name.

The same thinking applies to Dictionary though, and we don't call it VariantDictionary. On the other hand, there are also no Pool*Dictionary variants.

One practical aspect: there is no standard Array type with which it could collide, is there a very popular crate with it?

Regarding the conversions between types, is the goal to create some extension methods to help with the conversions?

Not necessarily, the conversions can be part of the type. Two things are important to me:

  • if one wants to do a certain conversion (let's say TRef -> Ref or Ref<T, Shared> -> Ref<T, Unique>), they know where to look
  • same operations/concepts are named the same in different places

Bromeon avatar Sep 27 '21 16:09 Bromeon

TypedArray -> PoolArray Would immediately reflect that it's the Rust counterpart of GDScript's Pool*Arrays. We need to decide if it makes sense to keep Int32Array and Float32Array around.

The aliases were mostly for backwards compatibility, and can be confusing to new users. It should be nice to remove them in favor of a singular PoolArray<T> interface.

chitoyuu avatar Nov 01 '21 20:11 chitoyuu

The remaining renames can be implemented during 0.10 (with deprecation) and/or for 0.11 (with removal). Will need more discussion anyway.

Bromeon avatar Dec 08 '21 19:12 Bromeon