-
-
Notifications
You must be signed in to change notification settings - Fork 200
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
Add ability to rename functions bound to Godot #376
Add ability to rename functions bound to Godot #376
Conversation
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-376 |
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.
Thanks a lot! This is a nice addition 🙂
I commented on a few things to simplify code flow. Some parts of it are related more to existing code that is now updated, rather than your code in particular.
Also thanks for the tests!
godot-macros/src/godot_api.rs
Outdated
// TODO you-win (August 8, 2023): might be worth it to be pub(crate) so that method registration can reuse the struct? | ||
struct GodotFn { | ||
func: Function, | ||
rename: Option<String>, | ||
} |
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 that refactoring is possible (method registration reusing the struct), could you do it right away?
Maybe name the struct Func
or FuncDefinition
or so, rather than GodotFn
.
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.
I have created a new FuncDefinition
struct in method_registration/mod.rs
func: Function, | ||
rename: Option<String>, | ||
} | ||
type Signal = Function; |
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.
I'd probably keep this as a separate type, as the possible options for #[func]
and #[signal]
likely diverge over time. It also slightly enhances type safety.
Signal renames are not yet supported as far as I can see, so they should not have this field.
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.
Is it not worth it to rename the type for a bit more clarity in the method signature for?
fn process_godot_fns(decl: &mut Impl) -> Result<(Vec<FuncDefinition>, Vec<Signal>), Error>
vs
fn process_godot_fns(decl: &mut Impl) -> Result<(Vec<FuncDefinition>, Vec<Function>), Error>
godot-macros/src/godot_api.rs
Outdated
name if name == "func" => { | ||
// TODO you-win (August 8, 2023): handle default values here as well? | ||
|
||
// Safe unwrap since #[func] must be present if we got to this point |
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.
The code flow gets a bit weird (not your code, but the result of the existing one growing).
We do this at the moment:
for attribute in all_attributes {
match attribute {
"func" => KvParser::parse(all_attributes) // again searching through all
"signal" => ...
}
}
detect_duplicates(); // custom validation that's less expressive than KvParser's built-in one
Whereas KvParser
is designed to already extract the relevant attributes and check for unhandled/duplicate/leftover ones in the finish()
call. So you could do this:
if let Some(mut parser) = KvParser::parse(attributes, "func")? {
// handle #[func]
// eventually:
parser.finish();
}
if let Some(_) = KvParser::parse(attributes, "signal")? {
// handle #[signal]
}
Do you think this could be done?
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.
This is in progress, my latest WIP code does not work properly since it picks up other annotations first.
Currently, it's picking up #[allow(clippy::uninlined_format_args)]
, but I will continue to poke at it.
* Modify func macro to accept a 'rename = ...' attribute * Add integration tests to cover renamed functions
e6480ef
to
be0ceee
Compare
…from a signature to resolve double immutable borrow
Replaced by #378. |
Comments
Please let me know if it would be better to break this functionality out like with field_var.rs. I tried to keep the number of changed files to a minimum.
There are also
TODO
s left in the file as a starting point for discussion, but feel free to start picking at the PR wherever you want.Still missing