Enable support for top level generic objects - #51
Conversation
| return false; | ||
| } | ||
|
|
||
| // Assume inner argument to type are valid. |
There was a problem hiding this comment.
why do we assume this? and not recursively invoke is_writeable?
There was a problem hiding this comment.
Not all generic arguments can be writable. For example HashMap<[u8; 4], f32>
There was a problem hiding this comment.
So we want this to return true for HashMap<[u8; 4], f32> even though it isn't writable? Why is that?
There was a problem hiding this comment.
I had issues not doing this. And I am still not completely certain what is_writable actually means anyway. Like. What even is it? Initially I had assumed it meant it was referencing an acceptable top level type for an object to be explicitly written out, but I now think it just means whether a path can or cannot be written in bauble entirely? If the former case, my logic here makes sense. If the latter, it does not.
I did initially validate the inner argument, but it did cause issues with the Bauble tests as I mentioned before. I think if you can clarify what you believe is_writable is exactly meant to indicate about a path and if it is the case it just means whether it can be written in bauble source, then yeah this code must validate the inner generic indeed!
There was a problem hiding this comment.
I think if you can clarify what you believe
is_writableis exactly meant to indicate about a path and if it is the case it just means whether it can be written in bauble source, then yeah this code must validate the inner generic indeed!
I'm not exactly sure either, I was somewhat hoping your response would clarify this for me! I've seen is_writeable used a lot for determining whether something is a sub-object or not (so not related to type paths), but there are a few other places where it is used that I don't fully understand. My assumption so far is that it both is meant to indicate whether a path can be written in bauble source and for whether a object path is a subobject.
It would be nice to pin down what is_writeable means so that we don't introduce bugs here.
| let mut generic_ending = false; | ||
| !self.is_empty() | ||
| && self.iter().all(|part| { | ||
| if generic_ending { |
There was a problem hiding this comment.
Does making generics writeable have any interaction with the built-in Ref<T> type?
There was a problem hiding this comment.
I am not sure honestly, I was unaware of this type. Can you elaborate?
There was a problem hiding this comment.
Whenever you have something referencing another object with the $path syntax bauble assigns it the type Ref<T> where T is the type of the referenced object. I think part of the is_writeable is meant to skip writing this type out when writing back to bauble source but I'm not fully sure.
E.g.
test = integration::Test { x: -5, y: 5 } // type integration::Test
test_ref = $test // type Ref<integration::Test>Concrete instances of the generic Ref<T> are created in get_or_register_asset_ref (called from ctx.register_asset())
There was a problem hiding this comment.
Ah, I was not aware of this. I am not sure why it would be an issue tbh, if Ref is registered in the context and given a path, why is it a problem if it is explicitly written out?
There was a problem hiding this comment.
Maybe it would be fine, although this depends on if it is registered before anything looking at the explicit path tries to look it up.
There was a problem hiding this comment.
I will look into this
Previously in Bauble, it was not possible to have generic top level objects. Example:
Was syntactically unsupported.
Bauble does generate distinct types for
Foo<Bar>, and so in order to support this it was simply necessary to allow Bauble to both parse explicit generic types and to resolve generic types withuse. This PR does both of these.