search

Arnavion / k8s-openapi

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

Improve listable ergonomics (#72) #94

Closed aquarhead closed 3 years ago

aquarhead commented 3 years ago

This currently will change Vec and BTreeMap types like this:

image

I'm pretty sure there're some rough edges though:

Also, can you add a rustfmt.toml that codifies the formatter style you use for this project? Even with auto format disabled it seems I still missed that existing code uses tabs rather than whitespaces and so on. It would really help contributors if the formatter just handles these.

Arnavion commented 3 years ago

Whether I missed some codegen path (I guess CI would catch those)

I mean, you don't need CI to catch them. You can try to cargo run the codegen and then try to cargo build the root crate yourself.

And if you did that, you'd notice that a) BTreeMap isn't in scope so this won't compile, and b) the structs don't use serde's custom derives so this won't compile either.

Also, can you add a rustfmt.toml that codifies the formatter style you use for this project?

I would, but rustfmt has one annoying feature that can't be configured last I checked. It insists that:

let some_very_long_foo = bar.some_very_long_baz();

should be formatted as:

let some_very_long_foo = bar
    .some_very_long_baz();

whereas I prefer:

let some_very_long_foo =
    bar
    .some_very_long_baz();
Arnavion commented 3 years ago

I've also just pushed a change to make it more obvious why CI failed on this PR.

aquarhead commented 3 years ago

Ah I did run the codegen but didn't attempt cargo build in root. Will address some obvious issues.

Arnavion commented 3 years ago

Or just run the tests, yes.

aquarhead commented 3 years ago

Last commit fixed various obvious issues, but cargo build still fails mostly due to as_ref() and ok_or_else calls on previously Option fields.

At this point I'm not sure whether this should continue... It seems like quite a breaking change and I'm not sure if I'm approaching this correctly?

Arnavion commented 3 years ago

Closing in favor of https://github.com/Arnavion/k8s-openapi/pull/95 . Feel free to review that one.