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.

Sign up for GitHub

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

Jump to bottom

Add a name optional argument to #[export] #734

Merged
merged 2 commits into from May 2, 2021
Merged

Add a name optional argument to #[export] #734

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
719e801
Add a `name` optional argument to `#[export]`
setzer22 May 1, 2021
b424d03
Address review feedback
setzer22 May 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
54 changes: 47 additions & 7 deletions gdnative-derive/src/methods.rs
View file
Open in desktop
Expand Up @@ -65,6 +65,7 @@ pub(crate) struct ExportMethod {
pub(crate) struct ExportArgs {
pub(crate) optional_args: Option<usize>,
pub(crate) rpc_mode: RpcMode,
pub(crate) name_override: Option<String>,
}

pub(crate) fn derive_methods(item_impl: ItemImpl) -> TokenStream2 {
Expand All @@ -82,7 +83,10 @@ pub(crate) fn derive_methods(item_impl: ItemImpl) -> TokenStream2 {
let sig_span = sig.ident.span();

let name = sig.ident;
let name_string = name.to_string();
let name_string = match args.name_override {
Some(name_override) => name_override,
None => name.to_string(),
};
Copy link

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better, thanks!

let ret_span = sig.output.span();
let ret_ty = match sig.output {
syn::ReturnType::Default => quote_spanned!(ret_span => ()),
Expand Down Expand Up @@ -181,6 +185,7 @@ fn impl_gdnative_expose(ast: ItemImpl) -> (ItemImpl, ClassMethodExport) {
ImplItem::Method(mut method) => {
let mut export_args = None;
let mut rpc = None;
let mut name_override = None;

let mut errors = vec![];

Expand Down Expand Up @@ -253,8 +258,9 @@ fn impl_gdnative_expose(ast: ItemImpl) -> (ItemImpl, ClassMethodExport) {
};
let path = last.ident.to_string();

// Match rpc mode
// Match optional export arguments
match path.as_str() {
// rpc mode
"rpc" => {
let value = if let syn::Lit::Str(lit_str) = lit {
lit_str.value()
Expand All @@ -281,17 +287,50 @@ fn impl_gdnative_expose(ast: ItemImpl) -> (ItemImpl, ClassMethodExport) {
));
return false;
}
}
// name override
"name" => {
let value = if let syn::Lit::Str(lit_str) = lit {
lit_str.value()
} else {
errors.push(syn::Error::new(
last.span(),
"unexpected type for name value, expected Str",
));
return false;
};

// 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
Copy link

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?

Copy link
Contributor Author

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.

match syn::parse_str::<syn::Ident>(value.as_ref()) {
Ok(_) => {
if name_override.replace(value).is_some() {
errors.push(syn::Error::new(
last.span(),
"name was set more than once",
));
return false;
}
}
Err(_) => {
errors.push(syn::Error::new(
last.span(),
"name value must be a valid identifier",
));
return false;
}
}
}
_ => {
let msg =
format!("unknown option for export: `{}`", path);
errors.push(syn::Error::new(last.span(), msg));
return false;
}
_ => (),
}

let msg = format!("unknown option for export: `{}`", path);
errors.push(syn::Error::new(last.span(), msg));
}
}

return false;
}
}
Expand Down Expand Up @@ -340,6 +379,7 @@ fn impl_gdnative_expose(ast: ItemImpl) -> (ItemImpl, ClassMethodExport) {

export_args.optional_args = optional_args;
export_args.rpc_mode = rpc.unwrap_or(RpcMode::Disabled);
export_args.name_override = name_override;

methods_to_export.push(ExportMethod {
sig: method.sig.clone(),
Expand Down