Skip to content
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

Rename symbols for increased clarity #815

Merged
merged 4 commits into from Nov 12, 2021
Merged

Conversation

Bromeon
Copy link
Member

@Bromeon Bromeon commented Nov 10, 2021

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

bors try

@Bromeon Bromeon added c: core Component: core (mod core_types, object, log, init, ...) breaking-change Issues and PRs that are breaking to fix/merge. quality-of-life No new functionality, but improves ergonomics/internals labels Nov 10, 2021
@Bromeon Bromeon added this to the v0.10 milestone Nov 10, 2021
bors bot added a commit that referenced this pull request Nov 10, 2021
@bors
Copy link
Contributor

bors bot commented Nov 10, 2021

try

Build failed:

@Bromeon
Copy link
Member Author

Bromeon commented Nov 10, 2021

Main controversy is probably ThreadAccess -> Ownership. It's not a very critical change, but was the result of my attempt to find some more or less orthogonal "policies" in the godot-rust type syste; that design anyway needs more thinking. Just some brainstorming in this message:

"Ownership" is a more common term than "thread access" and makes the concept of sharing Refs slightly clearer in my opinion. In godot-rust, this policy does not really specify who accesses (calls a method on) the object, but rather who owns (holds a reference to) it. In the case of Ref<T, Shared> there can be many live references which are just sitting around. Ownership is not only related to access, but also lifetime: it determines when ref-counted objects are destroyed.

On the other hand this term doesn't have "Thread" in the name, so thread-related ownership might not be the first association.

There are also multiple options to deal with generic type parameters vs. policy name:

// so far:
struct MyStruct<Access: ThreadAccess> {}
trait MyTrait { type RefKind: RefKind; } // reuse same name

// possible:
struct MyStruct<Ows: Ownership> {} // often-used type parameter short, makes some code more concise
trait MyTrait { type Mem: Memory } // or:
trait MyTrait { type Memory: Memory }

// or:
struct MyStruct<Ownership: OwnershipPolicy> {} // appears in docs as MyStruct<Ownership>
trait MyTrait { type Memory: MemoryPolicy } // or:
trait MyTrait { type Memory: Memory }

Especially since Access/ThreadAccess is such a common policy across many types in gdnative::object, a short name might help readability in more complex types. It could even become a convention that policy type parameters are 3-4 letters, to separate them from typical structs/traits.

pub fn try_from_base(owner: Ref<T::Base, Ows>) -> Result<Self, Ref<T::Base, Ows>>;
pub fn try_from_base(owner: Ref<T::Base, Ownership>) -> Result<Self, Ref<T::Base, Ownership>>;

But I agree it first looks weird because Ows isn't known. Although Access isn't self-declaring either; so in both cases one will become familiar with the identifier after reading a few docs.

bors bot added a commit that referenced this pull request Nov 10, 2021
@bors
Copy link
Contributor

bors bot commented Nov 11, 2021

try

Build succeeded:

@Bromeon Bromeon changed the title Qol/renames Rename symbols for increased clarity Nov 11, 2021
@chitoyuu
Copy link
Contributor

chitoyuu commented Nov 12, 2021

There are also multiple options to deal with generic type parameters vs. policy name:

// so far:
struct MyStruct<Access: ThreadAccess> {}
trait MyTrait { type RefKind: RefKind; } // reuse same name

// possible:
struct MyStruct<Ows: Ownership> {} // often-used type parameter short, makes some code more concise
trait MyTrait { type Mem: Memory } // or:
trait MyTrait { type Memory: Memory }

// or:
struct MyStruct<Ownership: OwnershipPolicy> {} // appears in docs as MyStruct<Ownership>
trait MyTrait { type Memory: MemoryPolicy } // or:
trait MyTrait { type Memory: Memory }

Especially since Access/ThreadAccess is such a common policy across many types in gdnative::object, a short name might help readability in more complex types. It could even become a convention that policy type parameters are 3-4 letters, to separate them from typical structs/traits.

I believe there is a convention in general to shorten type parameters, but keep the full trait name for associated types, so I'd prefer type Memory: Memory a bit more than the currently proposed type Mem. Shortening Ownership to something is fine, although imo the name Ows specifically might as well just be O. There aren't enough syllables in "ws" to help users figure out what the full word is supposed to be. The slightly longer Owns is more helpful in that regard, but is confusing since it collides with a real English word. I think it might be helpful to ask more people for their opinions before moving forward with the current naming of Ows.

Also change Memory type-tags to enums (non-inhabitable types, no need for derive)
@Bromeon Bromeon marked this pull request as ready for review November 12, 2021 19:43
@Bromeon
Copy link
Member Author

Bromeon commented Nov 12, 2021

Thanks for the feedback! Changed:

  • Ows -> Own
  • Mem -> Memory

I'll merge the current state of things to unblock other changes. The rest can be done in a separate PR.

bors r+

@bors
Copy link
Contributor

bors bot commented Nov 12, 2021

Build succeeded:

@bors bors bot merged commit deb9dbf into godot-rust:master Nov 12, 2021
@Bromeon Bromeon deleted the qol/renames branch November 12, 2021 19:58
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

Successfully merging this pull request may close these issues.

None yet

2 participants