k8s-openapi
k8s-openapi copied to clipboard
Implement deep merge
This is an alternative approach suggested in #120
TODO:
- [x] seems like due to using traits, I don't need all that extra type tracking - remove
- [x] validate in
rustshop
code - [x] cleanup spaces (why do you use tabs for Rust projects, BTW? :P)
- [x] revise some TODOs left in the code
I've rewrote the previous code I had to use this PR instead.
I had to import a macro for map literals, and with that it's not too bad. Lack of any auto-Option
conversion/insertion is a noticeable and annoying difference, but that's kind of it. Though some parts are even better (setting multiple fields on the same struct).
Once (if) std::default
stabilizes some of this Default::default
noise can be removed, though for now I can always just use default::default
.
Option::get_or_insert_default
will be a minor improvement too once it stabilizes.
I've fixed the whitespace errors, missing impl for serde_json::Value
, wrong reference to the trait in the templates, incorrect impls of the trait for borrowed types, etc etc in pr/121.
One thing that's broken is the custom derive, since the codegen of FooBar: DeepMerge
requires FooBarSpec: DeepMerge
, but that's a user type. I don't want to make users impl it by hand, at least not if they don't care for FooBar: DeepMerge
in the first place.
So the options seem to be:
-
Make it easy for all users (both those who want
FooBar: DeepMerge
and those who don't care for it) to implDeepMerge
forFooBarSpec
via a new custom derive. This is not straightforward because theSpec
type could be anything, eg a struct or enum with other arbitrary types. -
Emit
impl DeepMerge for FooBar where FooBarSpec: DeepMerge { ... }
, so that users who don't impl the trait for theirFooBarSpec
still have compilable code. But this requires#![feature(trivial_bounds)]
, so it's a no-go. -
Make the
impl DeepMerge for FooBar
codegen conditional on an attr of#[derive(CustomResourceDefinition)]
so that only users who opt in to it get it.
(3) seems to be the only path forward.
Just a status update: I've had much less time recently, and it looks like it will not change for a month or more, so probably won't be able to improve it anytime soon. The issues you describe seem a bit out of depth for me RN, so I would have to invest quite a bit of time just to start working on them.
Feel free to take over if you can/wish. I probably will get back to it if needs more work, once I find some time.
Yes, it's on my plate. I've just been busy myself.
Okay, I've updated pr/121 with the change to the custom derive to make the impl of DeepMerge
for the generated CR type opt-in. If you're okay with that branch, I'll merge it.
The other major changes are:
-
I removed the default impl of
merge_from
that did*self = other;
, since I want to make sure users implementing the trait on their own types do it "properly" (field-by-field). -
I changed the codegen for structs to do
DeepMerge::merge_from(&mut self.field, other.field)
instead ofself.field.merge_from(other.field)
because the field type could have an inherentfn merge_from
that would be chosen over the trait method. None of the structs in this crate have such an inherent method, but the user's CRD spec type could have one which would be problematic for the generated CR's impl. -
Changed the impl for
Vec
to doself.extend(other);
instead ofself.append(&mut other);
. They both forward to the same specialized function internally, so.extend
is clearer since it consumedother
instead of draining it.
My code seem to compile as is with this change and I have very minimal requirements. Everything that you described and I understood LGTM. :)