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.

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

Update type strings according to how godot 4.3 exports them currently #717

Merged
merged 1 commit into from
May 21, 2024

Conversation

lilizoey
Copy link
Member

Makes our exports match what godotengine/godot#90716 does

i am now wondering if we have some code-duplication with the type string hint impl, but i guess we can look into that at some other point

@lilizoey lilizoey added bug c: register Register classes, functions and other symbols to GDScript labels May 20, 2024
@lilizoey
Copy link
Member Author

oh i need to do runtime checks not compile time checks, that's annoying

@GodotRust
Copy link

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

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! I also started investigating this on RocketChat, but you were faster to come up with a working fix! 😀

Now we have the 4.3 differentiation logic in multiple places -- I wonder if we instead should just always pass the type name to PropertyHintInfo::with_hint_none() and do the differentiation there?

(Same for the type string)

}
}
};

(@type_string_hint $Ty:ty) => {
impl TypeStringHint for $Ty {
impl TypeStringHint for $Ty {
Copy link
Member

Choose a reason for hiding this comment

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

This indentation is probably a mistake.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, rustfmt isn't the best at formatting macros

@lilizoey
Copy link
Member Author

Thanks a lot! I also started investigating this on RocketChat, but you were faster to come up with a working fix! 😀

yeah i did notice something in the actions after id done the changes, but it didn't seem to have progressed very far. i think im just a bit more familiar with this part of the codebase since i think i made a lot of it.

Now we have the 4.3 differentiation logic in multiple places -- I wonder if we instead should just always pass the type name to PropertyHintInfo::with_hint_none() and do the differentiation there?

(Same for the type string)

maybe yeah, i think initially i was planning on having with_hint_none_type_name be deprecated in 4.3 but then i realized i needed runtime checks so that doesn't work

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.

Thank you! I might have a look at some point if we can also reduce duplication in type_string() (e.g. using another function), but that's less important. I'll merge to unblock the failing CI!

@Bromeon Bromeon added this pull request to the merge queue May 21, 2024
Merged via the queue into godot-rust:master with commit a9a57a5 May 21, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug c: register Register classes, functions and other symbols to GDScript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants