-
Notifications
You must be signed in to change notification settings - Fork 4
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
Simplify the whole Model serialization story #2
Conversation
I should mention there's one downside to this approach: the list of possible models is still closed. A user of the library won't be able to extend it with their own implementors. Does that matter? |
Codecov Report
@@ Coverage Diff @@
## main #2 +/- ##
==========================================
+ Coverage 78.87% 80.98% +2.11%
==========================================
Files 17 17
Lines 1704 1604 -100
==========================================
- Hits 1344 1299 -45
+ Misses 360 305 -55
Continue to review full report at Codecov.
|
I let myself push one more thing in here! I think it's better if the This does introduce a little boilerplate though with the extra implementation of |
This is awesome, @uint.
It would be wonderful if users could use "sim" as a standard Cargo dependency, but also use custom models that they write. We can't do this with the crate today, so there is no regression. However, making this easier to implement in the future is valuable. The enum dispatch approach is great. Is there any way that this solution could be adapted in the future to accommodate user-defined models? Could metaprogramming/macros maybe work? |
I don't think we can use macros for this. There's no supported way of collecting all trait implementations and using that list during macro expansion (to build an enum of all possible models). typetag does some magic (using the inventory crate) to construct that kind of list, but that list is a static var - which is also not usable during macro expansion, only in runtime. The way I see it there are two ways to make this extensible to Rustaceans using the library:
|
Thanks @uint. Option 2 sounds great - I don't expect the I'm not sure if you want to split this up into multiple PRs, or just use one - I have no preference. I'll just keep watch for anything new. |
I was hoping for option 2 as well. I always feel like trait objects can introduce some quirks, and that they're more of a "last resort" or "quick" solution. With Rust, I tend to prefer things that incur no runtime cost (or it's optional) and encode more information in the type system, to be statically analyzed by the compiler - if that makes sense? Feels Rustier. I think I'd prefer to do it in a separate PR, once this (or #7?) is merged - I like dividing work up into smaller chunks. I'm not picky though. |
I fully agree, I too prefer having things on static dispatch |
Thanks @uint and @smallstepman |
I initially called this an overhaul, but it's rather simple.
Introducing enum_dispatch helped get rid of some code, make things DRYer, resolve the issues you mentioned and probably make this faster by avoiding dynamic dispatch.