Arnavion / k8s-openapi

Rust definitions of the resource types in the Kubernetes client API
Apache License 2.0
386 stars 41 forks source link

Implement deep merge #121

Closed dpc closed 2 years ago

dpc commented 2 years ago

This is an alternative approach suggested in #120

TODO:

dpc commented 2 years ago

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.

Arnavion commented 2 years ago

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.

dpc commented 2 years ago

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.

Arnavion commented 2 years ago

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

Arnavion commented 2 years ago

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:

dpc commented 2 years ago

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