-
Notifications
You must be signed in to change notification settings - Fork 16
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
Remove TransformsFromConfig trait #278
Remove TransformsFromConfig trait #278
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This refactor makes sense to me. Nice one
This makes sense... though would it be better to actually just have the config as a dyn trait object and remove the enum? I think I've looked at this before and can't remember if it makes serialization of the config struct hard? Config is just done at startup... so no perf concerns |
I thought you couldnt serialize trait objects. |
I got a 95% solution for a config trait. However among various minor complications such as extra complexity + compile times the main problem comes down to a limitation in deserialization with typetag. So because of this lets stick with this PR and abandon using a trait. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit disapointing that typetag makes the UX worse as it looked like a simple solution to "registering" config objects.
This largely looks good to me.
Can you update the docs. cargo doc
throws some warnings for refs to the now non-existent config trait. The docs folder might also need updating (docs.shotover.io)
cargo doc changes should be good. |
1 benchmarks reported regressed performance. Please check the benchmark workflow logs for details: https://github.com/shotover/shotover-proxy/actions/runs/1366148068 |
Understanding how config works is easier without the extra layer of indirection.