kube-rs / kopium

Kubernetes OPenapI UnMangler
Apache License 2.0
112 stars 21 forks source link

Codegen for standard Condition structure broken when running with hide-prelude #214

Closed rossdylan closed 5 months ago

rossdylan commented 6 months ago

I recently upgraded kopium and noticed that my usage of --hide-prelude causes kopium to generate broken code. Specifically, the change in #203 always generates a Vec<Condition> which relies on Condition always being imported. While looking into fixing this I noticed that hide-prelude isn't really taken into account for any of the imports during codegen.

Figured I'd open an issue to sort out a good way forward here. In my mind there are a couple of options differing in the amount of effort required.

  1. Thread a hide_prelude option down into the analyzer (like no_condition) and use that to dispatch to two different import paths
  2. Add some kind of import resolver, maybe off of Config, that can be used to do that dispatch in a more generic way. (something like array_type = format!("Vec<{}>"), self.config.resolve_import("Condition"))
  3. Remove --hide-prelude entirely and always use absolute imports. I'm leaning towards this solution since it makes it easier to avoid other problems like this in the future, and also solves the noisy unused imports warning that --hide-prelude fixes.
clux commented 6 months ago

hm, yeah, appreciate the issue. Upon looking at this again, I kind of feel like the original intention of --hide-prelude (allow custom crate names) has slowly morphed into a flag that generates broken imports by design and forces the user create their own imports for everything instead - which is not great.

I don't think we should move towards absolute imports because there are genuine reasons to change where something came from that cannot currently be changed in another way:

(not all of these are super practical atm (and there is another issue for customizing the map type i believe), but this allows a large degree of control.)


my gut feel is that it's probably easier if we remove/change --hide-prelude into something smarter (like --hide-import Condition) where you explicitly opt-in to the stuff you want to override the absolute path for (can make an enum for the print_prelude fn and the multiple-value clap arg). that way, we are less likely to break your existing ci scripts when adding support for a new thing.

rossdylan commented 6 months ago

That makes sense to me, I don't think I've got time to help implement, but happy to test it out when it lands!