Support @export_node_path attribute#1620
Conversation
de6a99d to
e2d03ab
Compare
|
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1620 |
Bromeon
left a comment
There was a problem hiding this comment.
Thanks a lot, this is a nice addition! 👍
PR looks mostly good, added some comments on smaller parts.
| // Detect parenthesized list: node_path = ("Button", "TouchScreenButton") | ||
| if tokens.len() == 1 | ||
| && let TokenTree::Group(group) = &tokens[0] | ||
| && group.delimiter() == Delimiter::Parenthesis | ||
| { | ||
| let list_parser = ListParser::new_from_tree( | ||
| tokens.into_iter().next().unwrap(), | ||
| Delimiter::Parenthesis, | ||
| )?; |
There was a problem hiding this comment.
Is this hand-parsing of tokens really needed before you pass it to ListParser?
How do other godot-rust proc-macro APIs that deal with lists do it?
There was a problem hiding this comment.
I've been breaking my head a bit with this today. I'm not using the handle_list(), since I thought, we would allow
[Option 1]
// ...
#[export(node_path = "Class1")] // (not a list)
// ...as a valid case.
Otherwise I get the error:
...
error: expected list of items
--> /Users/carlosfernandez/Git/gdext/target/debug/build/itest-c56514b7f101be8d/out/gen_ffi.rs:2412:30
|
2412 | #[export(node_path = "Button")]
| ^^^^^^^^
...
If we were to parse this only as a list, the above wouldn't work, and instead we would need to only allow:
[Option 2]
// ..
#[export(node_path = ("Class1"))]. // (list with only one entry)
// ...
}I'm removing the [Option 1] for now though, since it's more in line with what is done in GDScript.
There was a problem hiding this comment.
Do we need both? I guess the one with 1 argument is the common one?
There was a problem hiding this comment.
I left [Option 2] (#[export(node_path = ("Class1", ...))]) in. I think it's indeed more in line with what users could use, plus allows for more freedom/flexibility. Also, I will mark this as resolved unless you (or anyone else) is really against this, although I personally like the freedom plus it's the code is as easy to maintain as the other handle_list() branches :)
There was a problem hiding this comment.
Please don't resolve conversations you still expect me to read, it's rather hard to locate them 🙂
But sounds good for now, we can always add the other syntax later.
There was a problem hiding this comment.
My bad 😅.
So I will leave it as is :)
e2d03ab to
86b6b08
Compare
Added support for
export_node_pathannotation (Completes #1128)Adds the ability to annotate fields with
@export_node_path:Note: I didn't find the exact PR where
export_node_pathwas introduced to GDScript upstream, so if it's needed let me know, and I will dig deeper. Otherwise let me know if I should adjust any of these changes.Couldn't have done it without other PRs like those of @Swivelgames on #1183. It really made it a lot easier to have that as reference, so thanks to them a lot!