-
-
Notifications
You must be signed in to change notification settings - Fork 217
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
#[base]
attribute is no longer necessary
#577
Conversation
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-577 |
How does it work with different field names? Does the "hint" semantics handle this? |
@0awful From my understanding of the source and description I believe the name is irrevelant, only the |
// deprecated #[base] | ||
if KvParser::parse(&named_field.attributes, "base")?.is_some() { | ||
has_deprecated_base = true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to have an explicit deprecation warning for this, why not make it work like it used to at least until we remove the deprecation warning? As is this deprecation warning doesn't help that much if your #[base]
declaration is incompatible with the new way of finding base fields (like say with a type alias).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In practice, most existing code will keep working due to it using Base<T>
already. But it should not be too hard to make #[base]
have the same semantics as #[hint(base)]
, I can do that. Thanks 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated, also quickly tested it by replacing one of the #[hint(base)]
with #[base]
in integration tests.
Changes
can now be written as:
The rationale is to reduce clutter and repetition (
#[base] base: Base<Self::Base>
😨), while still functioning in almost all cases. It also follows prior art where non-annotated items of a proc macro are inspected without being annotated:OnReady<T>
type inference (exactly like here)#[class(init)]
default implementation#[godot_api] impl I... for MyClass
blocksWith the convention of using
base
as the field name (although this is still not required) and the necessity to useBase<T>
as its type, I don't think code becomes less readable. Imo the magic budget for this change is justifiable, as there is little potential for confusion and the remaining code is valid Rust, just not wired up.Furthermore, it is no longer possible to accidentally forget the
#[base]
attribute. This was previously possible as long as the field was not accessed (with only unused-code warning, which is not uncommon during gamedev prototyping).An alternative considered was to keep
#[base]
, but reduce repetition on the type, i.e.#[base] base: Node
. The problem is that derive macros cannot modify their output, so#[derive(GodotClass)]
would need to be converted into an attribute macro, which comes with its own share of implications.Compatibility
In typical fashion for godot-rust, existing code that uses
#[base]
keeps working but now gets a warning.The attribute itself becomes deprecated and will be removed in the future.
Fine-grained control
Magic inference should work in 99% of cases, but to handle the 1%, you can help the proc-macro with hints, which allow overriding the inference with explicit intent.
The new
#[hint]
attribute also supportsonready
andno_onready
keys; something that wasn't possible to configure before.