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 a name
optional argument to #[export]
#734
Conversation
This allows exporting methods to godot under a different name. Implements godot-rust#732
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 few comments about the code. Please see if any applies.
gdnative-derive/src/methods.rs
Outdated
let name_string = match args.name_override { | ||
Some(name_override) => name_override, | ||
None => name.to_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.
Could be written more concisely as args.name_override.unwrap_or_else(|| name.to_string())
, I believe?
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.
Much better, thanks!
gdnative-derive/src/methods.rs
Outdated
// NOTE: We take advantage of rust identifiers following | ||
// the same syntax rules as Godot method identifiers. See: | ||
// https://docs.godotengine.org/en/stable/getting_started/scripting/gdscript/gdscript_basics.html#identifiers |
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 wonder if it's really required that a method name is a valid identifier. We know for sure that properties can have non-identifier names: the property name of the controller of an AnimationTree
is "parameters/playback"
, for example. I guess a non-identifier name can still be called using call()
? If so, is it still necessary to check this here?
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've just checked and indeed exported methods don't need to be valid identifiers, so I've lifted the restriction and now any string can be an exported method.
@toasteater Just made a new commit addressing your comments. Let me know if there's anything else! |
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.
Looks good to me! It might make sense to keep the identifier-checking code in a separate commit, though in general it would make the history cleaner if the commits are squashed. Thanks for the PR!
bors r+
Build succeeded: |
This allows exporting methods to godot under a different name.
This commit implements the feature described in #732