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

#[derive(Property, Export)] for enums #371

Merged
merged 1 commit into from
Aug 6, 2023
Merged

#[derive(Property, Export)] for enums #371

merged 1 commit into from
Aug 6, 2023

Conversation

patataofcourse
Copy link
Contributor

@patataofcourse patataofcourse commented Aug 3, 2023 •
edited
Loading

MVP macros for deriving Property and Export in enums.

Not done yet, but hoping for reviews since this is my first contribution to a library of this size.

Commits will be squashed later, please review as one for now.

Current requirements for deriving these traits:

  • Must be an enum
  • Must have an explicit #[repr(i*/u*)] type
  • Variants must be unit/fieldless and have explicit discriminant numbers (A = 2 instead of A(u32) = 2 or A)

From earlier attempts, it seemed like ToVariant and FromVariant were needed as well? Doesn't seem to be the case anymore, weirdly.

TODO list:

  • #[derive(Property)]
    • Implement
    • Create tests
    • Document
    • Expand support?
  • #[derive(Export)]
    • Implement
    • Create tests
    • Document
    • Expand support?

@Bromeon Bromeon added feature Adds functionality to the library c: register Register classes, functions and other symbols to GDScript labels Aug 3, 2023
@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-371

Bromeon
Bromeon reviewed Aug 3, 2023
View reviewed changes
Copy link
Member

@Bromeon Bromeon left a 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 this! 👍

Really nice that you even added generic support! Do you think it's possible to list one generic example in the test as well, to make sure the syntax is parsed correctly and works?

godot-macros/src/derive_property.rs Outdated Show resolved Hide resolved
godot-macros/src/derive_property.rs Outdated Show resolved Hide resolved
godot-macros/src/derive_property.rs Outdated Show resolved Hide resolved
godot-macros/src/derive_property.rs Outdated Show resolved Hide resolved
godot-macros/src/derive_property.rs Outdated Show resolved Hide resolved
godot-macros/src/derive_property.rs Outdated Show resolved Hide resolved
itest/rust/src/property_test.rs Outdated Show resolved Hide resolved
itest/rust/src/property_test.rs Outdated
Comment on lines 314 to 316
assert_eq!(class.get_foo(), Test::B as i64);
class.set_foo(Test::C as i64);
assert_eq!(class.foo, Test::C);
Copy link
Member

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 possible that get_foo/set_foo return/accept directly the enum in Rust, not the i64?

Although maybe that's more complex; in that case it could be done later as a QoL improvement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The scope of this PR is more to add the proc macros than to modify library code - I'm not saying I won't be doing this or that it's completely outside the scope of this PR, but this is definitely not a priority for me rn.

@patataofcourse
Copy link
Contributor Author

Aside from the stuff I resolved in the reviews (if it's not resolved feel free to reopen), I removed generic support (since it's not usable in any way right now).
I also changed some other stuff then immediately reverted it because it broke. oops.

Bromeon
Bromeon reviewed Aug 3, 2023
View reviewed changes
godot-macros/src/derive_property.rs Outdated Show resolved Hide resolved
godot-macros/src/derive_property.rs Show resolved Hide resolved
godot-macros/src/derive_property.rs Outdated Show resolved Hide resolved
godot-macros/src/derive_property.rs Outdated Show resolved Hide resolved
godot-macros/src/derive_property.rs Outdated Show resolved Hide resolved
@patataofcourse patataofcourse marked this pull request as ready for review August 3, 2023 21:28
@Bromeon Bromeon mentioned this pull request Jun 14, 2023
Open
Bromeon
Bromeon reviewed Aug 3, 2023
View reviewed changes
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Some superficial comments about the new changes, will probably need another look tomorrow...

godot-macros/src/lib.rs Outdated Show resolved Hide resolved
godot-macros/src/derive_export.rs Outdated Show resolved Hide resolved
godot-macros/src/lib.rs Outdated Show resolved Hide resolved
godot-macros/src/lib.rs Outdated Show resolved Hide resolved
godot-macros/src/lib.rs Outdated Show resolved Hide resolved
Bromeon
Bromeon reviewed Aug 5, 2023
View reviewed changes
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Thanks a lot! The CI fails due to memory leaks, see first comment for addressing them.

After addressing the remaining comments and green CI, it should be good to go, so you could already squash the commits!

itest/rust/src/property_test.rs Outdated Show resolved Hide resolved
godot-macros/src/lib.rs Show resolved Hide resolved
godot-macros/src/lib.rs Outdated Show resolved Hide resolved
@patataofcourse
\#[derive(Property, Export)]
e44194a 
Initial work for #[derive(Property)] (failing)

general fixing around

make proper tests for derive(Property)

review stuff tm

remove default repr type since isize isnt GodotFfi

this is what happens when i try to commit too fast

again

Documentation + review fixes

implement and document Export

add a test for derive(Export)

review fixing

ci fixes

further fixing

oopsies
@Bromeon Bromeon added this pull request to the merge queue Aug 6, 2023
@Bromeon
Copy link
Member

Bromeon commented Aug 6, 2023

Thank you! 👍

Merged via the queue into godot-rust:master with commit 7a44ea8 Aug 6, 2023
13 checks passed
@patataofcourse patataofcourse deleted the derive_property_export branch August 6, 2023 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Bromeon Bromeon Bromeon left review comments

Assignees
No one assigned
Labels
c: register Register classes, functions and other symbols to GDScript feature Adds functionality to the library
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@patataofcourse @GodotRust @Bromeon