-
-
Notifications
You must be signed in to change notification settings - Fork 196
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 as_str
and godot_name
to non-bitfield enums
#898
Conversation
ffc3ff4
to
1213e22
Compare
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-898 |
14c1209
to
b28bbcb
Compare
Sorry about that, was having trouble using clippy on my end, but this should pass now. |
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.
No worries, and thank you for your contribution 😊
Could you add a short integration test in enum_test.rs? You can use some existing enums that are already generated in the minimal codegen.
If you find a case where Rust/Godot have same enumerator names, even better 🙂
godot-codegen/src/generator/enums.rs
Outdated
// Only enumerations with different godot names are specified. | ||
// `as_str` is called for the rest of them. | ||
let godot_name_enumerators = { |
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.
// Only enumerations with different godot names are specified. | |
// `as_str` is called for the rest of them. | |
let godot_name_enumerators = { | |
// Only enumerations with different Godot names are specified. | |
// `as_str` is called for the rest of them. | |
let godot_different_cases = { |
Reason for name is that
- you're specifying match arms (cases), not enumerators
- highlight that only different names are listed
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.
General question, does this happen a lot? Most Godot enumerators are prefixed with their enum type, while Rust ones aren't 🤔
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.
In terms of differences outside of just the prefix, as far as I know there aren't any. I thought it would be a nice feature to have, but in retrospect maybe it would be better to have a function that just returns the Godot prefix instead? 😅
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.
It's not as simple as "Rust ident == Godot prefix + enumerator name", the algorithm to map is quite complex and needs to handle some special cases. As such, a prefix would not be useful.
I checked quickly:
- 4724 enumerators total in the API
- 4565 (96.6%) of which differ between Godot and Rust
- 159 (3.4%) of which remain the same between Godot and Rust
Despite the minority, I think it doesn't hurt much to reuse as_str()
where possible. If it turns out to be negative in any way, we can always change the implementation.
b28bbcb
to
44f38db
Compare
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 for your contribution!
As far as I can tell, you can only get the name of an enumeration from
Debug
, and it's impossible to get the Godot name. This is possible in GDScript, and would be a nice feature to have (in my case I'd like to load keyboard prompts icons based on theKey
enum name).This PR extracts the
match
statement generated forDebug
intoas_str
so you can get the names with less overhead. And it also addsgodot_name
which returns the GDScript version of the name if desired.