Skip to content

Naming inconsistencies #773

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

Open
3 of 5 tasks
Bromeon opened this issue Aug 27, 2021 · 4 comments
Open
3 of 5 tasks

Naming inconsistencies #773

Bromeon opened this issue Aug 27, 2021 · 4 comments
Labels
breaking-change Issues and PRs that are breaking to fix/merge. c: core Component: core (mod core_types, object, log, init, ...) quality-of-life No new functionality, but improves ergonomics/internals
Milestone

Comments

@Bromeon
Copy link
Member

Bromeon commented Aug 27, 2021

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.

  • 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.

  • 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.

  • 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 Bromeon added feature Adds functionality to the library c: core Component: core (mod core_types, object, log, init, ...) breaking-change Issues and PRs that are breaking to fix/merge. labels Aug 27, 2021
@Bromeon Bromeon added this to the v0.11 milestone Aug 27, 2021
@jacobsky
Copy link
Contributor

@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?

@Bromeon
Copy link
Member Author

Bromeon commented Sep 27, 2021

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 Bromeon added quality-of-life No new functionality, but improves ergonomics/internals and removed feature Adds functionality to the library labels Oct 30, 2021
@Bromeon Bromeon modified the milestones: v0.11, v0.10 Nov 1, 2021
@chitoyuu
Copy link
Contributor

chitoyuu commented Nov 1, 2021

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.

bors bot added a commit that referenced this issue Nov 12, 2021
815: Rename symbols for increased clarity r=Bromeon a=Bromeon

Addressing the first few of the pending renames in #773.
Changes are separated by commits.

bors try


Co-authored-by: Jan Haller <[email protected]>
@Bromeon
Copy link
Member Author

Bromeon commented Dec 8, 2021

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

@Bromeon Bromeon modified the milestones: v0.10.0, v0.10.1 Dec 8, 2021
@Bromeon Bromeon modified the milestones: v0.10.1, v0.11 Jul 16, 2022
@chitoyuu chitoyuu modified the milestones: unplanned, v0.13.0 Jan 9, 2023
bors bot added a commit that referenced this issue Jan 9, 2023
1006: Expose RPC modes for properties r=chitoyuu a=chitoyuu

This adds:

- The `PropertyBuilder::with_rpc_mode` method
- The `#[property(rpc = "mode")]` syntax for the NativeClass macro

Close #995

1007: Deprecate specialized pool array aliases r=chitoyuu a=chitoyuu

Related: #773

Co-authored-by: Chitose Yuuzaki <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Issues and PRs that are breaking to fix/merge. c: core Component: core (mod core_types, object, log, init, ...) quality-of-life No new functionality, but improves ergonomics/internals
Projects
None yet
Development

No branches or pull requests

3 participants