-
-
Notifications
You must be signed in to change notification settings - Fork 231
Add groups export. #1214
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: master
Are you sure you want to change the base?
Add groups export. #1214
Conversation
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1214 |
- Edit registered_docs.xml – from now on `export`ed fields are being documented after `#[var]` fields.
Looks like the docs are just using alphabetical order? |
Coincidence – "a" is after "b" while all the items are not a it went from:
for gdext/itest/rust/src/register_tests/register_docs_test.rs Lines 156 to 179 in e1073bc
to
I can change this order and document this quirk 🤔 EDIT: ah, now I see – you meant GODOT docs, not the test ones 😅 – it seems you are right (I checked it with other examples and yeah, they are sorted alphabetically). |
- Formatting fixes, use `sort_by` instead of `sort_unstable_by` (as the already created comment states).
Properties are implicitly added to the most recently defined group, like in GDScript, right? I'm not a fan of attributes having implicit effects on other properties. |
@@ -749,6 +737,7 @@ fn parse_fields( | |||
} | |||
|
|||
Ok(Fields { | |||
groups, |
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 you call format_groups
here and pass the Vec by value? fn format_groups(groups: Vec<String>) -> Vec<String>
pub(crate) fn new_from_kv(parser: &mut KvParser, groups: &mut Vec<String>) -> Self { | ||
Self { | ||
group_name_index: Self::parse_group("group", parser, groups), | ||
subgroup_name_index: Self::parse_group("subgroup", parser, groups), | ||
} | ||
} | ||
} |
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 currently accepts any token-tree and just prays that it's a string litteral. This is also accepted:
#[export(group = f64::MAX)]
but is turned into a group "f64 :: MAX"
which is not something I'd expect.
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 would argue that it might be desirable, bigger issue might be typo such as #[export(group = g1765 range = (0.0, 10.0, or_greater))]
(lack of comma) 😄
What does this PR solve?
Adds groups and subgroup.
How does it work?
I checked few options and finally went with a
clap
API (suggested by @ttencate) which requires specifying the group directly inside a#[export]
attribute – everything else turned out to be too limiting, too messy or too janky 😒.Example usage
Problems etc
Initially I wanted to ship it with struct extensions but they can be added in separate PR (I'll create the issue for that later). The leftovers of that is
Fields
struct being moved to separate file – I don't see anything wrong with that though, so I left it like that (not sure aboutnamed_fields
function though).I experimented with organizing fields into something along the lines of
but it turned out to be silly, unnecessary and unnecessary overcomplicated.
I am not really happy with
GroupIdentifier
(usize
which simply points to some index holding group names) but it is kinda the sanest solution – idents make no sense (and use "to_string()" underneath anyway), strings require too many unnecessary allocations,*const String
is… uh… let's not do that.