Add Array typehints #639
Add Array typehints #639
Conversation
| /// Need the Hint trait so that nested Array hints work. | ||
| pub trait Hint: fmt::Debug { | ||
| fn export_info(&self) -> ExportInfo; | ||
| } | ||
|
|
||
| impl Hint for () { | ||
| fn export_info(&self) -> ExportInfo { | ||
| panic!("Unit hint should never be called, only used as a hintless placeholder."); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This can be avoided by using Export as the trait bound in ArrayHint::with_subhint instead.
| /// Hints for Array, allowing nesting | ||
| #[derive(Debug)] | ||
| pub struct ArrayHint { | ||
| element_type: Option<Box<dyn Hint>>, |
There was a problem hiding this comment.
The boxing is unnecessary because you can just store an ExportInfo here.
| ArrayHint { element_type: None } | ||
| } | ||
|
|
||
| pub fn with_subhint<T>(hint: T) -> Self |
There was a problem hiding this comment.
Perhaps with_element_hint would be a better name for this?
| ( | ||
| VariantType::VariantArray, | ||
| sys::godot_property_hint_GODOT_PROPERTY_HINT_TYPE_STRING, | ||
| ) => format!("19:{}", subinfo.hint_string), |
There was a problem hiding this comment.
Is this 19 value exported as a constant somewhere in the C API? If so, would it be better to use that constant instead of a hardcoded magic number?
|
All good points. I've tried to address them here. Still untested. I'll set aside some time to test them tonight, in 10 hours or so. |
|
I've set up a simple example that looks to be working as expected for me. So far, it just sets some simple properties of single and double arrays with and without hints. |
| /// Need the Hint trait so that nested Array hints work. | ||
| pub trait Hint: fmt::Debug { | ||
| fn export_info(self) -> ExportInfo; | ||
| } | ||
|
|
||
| impl Hint for () { | ||
| fn export_info(self) -> ExportInfo { | ||
| panic!("Unit hint should never be called, only used as a hintless placeholder."); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
If hint types remain as associated types, is this trait still necessary?
| pub fn with_element_hint<T>(hint: T) -> Self | ||
| where | ||
| T: Hint, | ||
| { |
There was a problem hiding this comment.
I don't think this can be used to export arrays of resources, because Ref<T> has the hint type of (). The constructor would just panic with the current implementation, and would not be able to know which type it's supposed to export even if that is changed.
ghost
added
the
ghost
|
Since the PR is being inactive, I have taken the liberty to make some changes and squash the commits. Since this is technically a breaking change (changing the In the future, we might want to make the "no hint available" hint type a "never" type, so that users will have to use |
|
Ah, thanks. Sorry this one slipped through the cracks. I meant to clean it up but it fell entirely off my radar. |
|
Build succeeded: |
This is still completely untested yet.
This draft PR is for code change review and discussion, potentially addressing (or partially addressing at least) #638
Some of it may be able to be trimmed down, made clearer, or better expressed. For instance, I'm not completely thrilled with ArrayHint, and I think there might be a better way around that with generics, but it would possibly involve some API change, as then VariantArray couldn't use the generic version to export due to needing to be able to implement Export with multiple Hint types.