k8s-openapi icon indicating copy to clipboard operation
k8s-openapi copied to clipboard

Implement deep merge

Open dpc opened this issue 1 year ago • 4 comments

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

dpc avatar Jul 16 '22 06:07 dpc

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.

dpc avatar Jul 17 '22 05:07 dpc

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:

  1. Make it easy for all users (both those who want FooBar: DeepMerge and those who don't care for it) to impl DeepMerge for FooBarSpec via a new custom derive. This is not straightforward because the Spec type could be anything, eg a struct or enum with other arbitrary types.

  2. Emit impl DeepMerge for FooBar where FooBarSpec: DeepMerge { ... }, so that users who don't impl the trait for their FooBarSpec still have compilable code. But this requires #![feature(trivial_bounds)], so it's a no-go.

  3. 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.

Arnavion avatar Jul 22 '22 12:07 Arnavion

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.

dpc avatar Jul 29 '22 18:07 dpc

Yes, it's on my plate. I've just been busy myself.

Arnavion avatar Jul 29 '22 18:07 Arnavion

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 of self.field.merge_from(other.field) because the field type could have an inherent fn 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 do self.extend(other); instead of self.append(&mut other);. They both forward to the same specialized function internally, so .extend is clearer since it consumed other instead of draining it.

Arnavion avatar Aug 10 '22 07:08 Arnavion

My code seem to compile as is with this change and I have very minimal requirements. Everything that you described and I understood LGTM. :)

dpc avatar Aug 10 '22 18:08 dpc