EmbarkStudios / mirror-mirror

🪞 Powerful reflection library for Rust
Apache License 2.0
79 stars 2 forks source link

Is `Type` required? #77

Closed bnjbvr closed 7 months ago

bnjbvr commented 1 year ago

It seems to me that the use-case for Type is:

It seems that Type also normalizes some information exposed via methods that can be useful on their own, e.g. default_value, type_name, etc.

So as to lower the API surface, I was wondering if Type could be removed, and replaced directly by the more precise type kinds?

As far as I can tell, reimplementing the "normalizing" methods would vary in difficulty:

If we went down that road, I'd also suggest suffixing all the more-precise-type-kinds with Descriptor, for symmetry with TypeDescriptor, as it seems this is what they are (?). (Maybe all of this could be shortened to the Desc suffix, in general, to not make for really long names like TupleStructTypeDescriptor.)

What do you think about this?

davidpdrsn commented 1 year ago

So as to lower the API surface, I was wondering if Type could be removed, and replaced directly by the more precise type kinds?

I'm not sure. I think having an enum to match on is very convenient. Would the alternative be

if let Some(struct_type) = type_descriptor.as_struct() {
    // its a struct!
}

if let Some(tuple_struct_type) = type_descriptor.as_tuple_struct() {
    // its a tuple struct
}

// repeat for tuple, enum, list, etc

If we went down that road, I'd also suggest suffixing all the more-precise-type-kinds with Descriptor, for symmetry with TypeDescriptor, as it seems this is what they are (?)

I think that would make the API too verbose. To me StructTypeDescriptor isn't really better than StructType, just longer.

This is a very personal opinion but in general I'd rather put related types in the same module, rather than use suffixes. Suffixes feel very "java" to me 🤷

davidpdrsn commented 1 year ago

I also think keeping into_type_descriptor is worth it. Being able to drill down into some deeply nested type and convert your destination type into an owned+serializable type seems useful to me. Even if we don't happen to use it ourselves yet.

bnjbvr commented 1 year ago

Alright, these are good arguments. Wonder if we could fuse into the other direction? TypeDescriptor would be an enum, and it would use a Cow<Graph> for the type graph, since it can either be owned (for the root element) or borrowed (for children type desc).

davidpdrsn commented 1 year ago

Yes that sounds like a good direction to try! I agree that getting rid of Type would make things cleaner overall.

davidpdrsn commented 7 months ago

I think we should just keep Type for now. Having used mirror-mirror a lot internally I don't think its too bad.