Skip to content

Rename: OnEditor invalid to sentinel #1079

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

Merged
merged 1 commit into from
Mar 16, 2025
Merged

Conversation

ColinWttt
Copy link
Contributor

@ColinWttt ColinWttt commented Mar 13, 2025

#[init(invalid= val)] -> #[init(sentinel = val)]
OnEditor::new_invalid(val) -> OnEditor::from_sentinel(val)

@Bromeon
Copy link
Member

Bromeon commented Mar 13, 2025

Thanks!

As mentioned in #1051 (review), "invalid" is not yet the final name -- we should probably decide on one and then adapt it everywhere. Maybe we can use this PR for some suggestions 🤔

Some ideas I had a while ago:

I'm not really sure about the name "uninit" in OnEditor::unit() + #[init(uninit = ...)]. First, people probably know this from mem::MaybeUninit::uninit(), where it has a totally different meaning (actually uninitialized memory).

Second, it doesn't express that the value is not only "uninitialized", but also a "invalid marker" for value that's used to distinguish valid/invalid state. In programming, sentinel value probably comes close to describe it.

I don't really have a great alternative though, maybe we can brainstorm a bit?

  • Single word: unset, invalid, ...
  • Marker: invalid_marker, uninit_marker
  • Sentinel: uninit_sentinel, ...
  • Descriptive: needs_init
  • Exclusion: except, exclude, excluded, exempt, unless...

(Perfect would be a single word expressing both "needs initialization" and "marks an invalid value", but that's very specific...)

My personal favorites:

#[init(sentinel = 7)] // 'sentinel value' is a semi-known term

OnEditor::uninit_sentinel(7) // naming?
#[init(unless = 7)] // consider initialized, unless it's 7

OnEditor::init_unless(7)

It doesn't seem like there's a simple enough term that makes the semantics 100% clear, so reading docs will be necessary anyway. That considered, distinction from other well-known concepts ("uninit" like MaybeUninit::uninit, or "invalid" like Plane::invalid) reduces confusion.

@ColinWttt
Copy link
Contributor Author

ColinWttt commented Mar 13, 2025

Second, it doesn't express that the value is not only "uninitialized", but also a "invalid marker" for value that's used to distinguish valid/invalid state. In programming, sentinel value probably comes close to describe it.

Sentinel looks pretty cool. As a non-native English speaker, I prefer some simple words like dummy, placeholder, and fake.

@Yarwin
Copy link
Contributor

Yarwin commented Mar 16, 2025

I think sentinel is way to go 👍, albeit dummy is a bit more descriptive (it literally stands for "dummy value displayed in the editor")

Personally I would go with OnEditor::with/new_sentinel(val) as stated in RFC-0430: finalizing-naming-conventions.

@Bromeon
Copy link
Member

Bromeon commented Mar 16, 2025

In OnReady, we use different constructors though:

  • new
  • node
  • manual
  • from_base_fn

So with_sentinel would be yet another convention.
Maybe OnReady ones should at some point be updated; node could probably be from_node.

Regarding this pull request, what about OnEditor::from_sentinel?

#[init(sentinel = 7)] 

OnEditor::from_sentinel(7)

rename invalid to sentinel

`new_invalid` to `from_sentinel`
@ColinWttt ColinWttt changed the title Docs: fix typo Rename: OnEditor invalid to sentinel Mar 16, 2025
@Bromeon Bromeon added quality-of-life No new functionality, but improves ergonomics/internals c: core Core components labels Mar 16, 2025
@Bromeon Bromeon added this pull request to the merge queue Mar 16, 2025
@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1079

Merged via the queue into godot-rust:master with commit ab08cd2 Mar 16, 2025
17 checks passed
@Bromeon
Copy link
Member

Bromeon commented Mar 16, 2025

Thanks a lot, and congrats to your first merged PR! 😊

@ColinWttt ColinWttt deleted the patch-1 branch March 18, 2025 03:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: core Core components quality-of-life No new functionality, but improves ergonomics/internals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants