Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make --derive more flexible #237

Merged
merged 21 commits into from
May 1, 2024
Merged

Make --derive more flexible #237

merged 21 commits into from
May 1, 2024

Conversation

MathiasPius
Copy link
Contributor

@MathiasPius MathiasPius commented Apr 30, 2024

Fixes #231

This overhauls the --derive flag in a backwards compatible way, allowing users to specify traits to derive on a per-object basis, on all structs, all enums, or just unit-only enums.

From the updated help text:

-D, --derive

Derive these additional traits on generated objects

There are three different ways of specifying traits to derive:

  1. A plain trait name will implement the trait for all objects generated from the custom resource definition: --derive PartialEq

  2. Constraining the derivation to a singular struct or enum: --derive IssuerAcmeSolversDns01CnameStrategy=PartialEq

  3. Constraining the derivation to only structs (@struct), enums (@enum) or unit-only enums (@enum:simple), meaning enums where no variants are tuple or structs: --derive @struct=PartialEq, --derive @enum=PartialEq, --derive @enum:simple=PartialEq

See also: https://doc.rust-lang.org/reference/items/enumerations.html

I'm still not entirely sure if the enum:simple syntax is the right way to go. I experimented with inverting it so enum matched only the simple enums, and you had to do enum+complex to trigger derivation for complex types, but that was even less intuitive since you would expect enum to cover all enums by default.

Signed-off-by: Mathias Pius <contact@pius.io>
…enums with only simple members.

Signed-off-by: Mathias Pius <contact@pius.io>
Fixes #231

Signed-off-by: Mathias Pius <contact@pius.io>
…ly enums

Signed-off-by: Mathias Pius <contact@pius.io>
Signed-off-by: Mathias Pius <contact@pius.io>
Signed-off-by: Mathias Pius <contact@pius.io>
Signed-off-by: Mathias Pius <contact@pius.io>
@MathiasPius
Copy link
Contributor Author

Seem to have lost some code during a rebase, hang tight!

Signed-off-by: Mathias Pius <contact@pius.io>
Signed-off-by: Mathias Pius <contact@pius.io>
Signed-off-by: Mathias Pius <contact@pius.io>
@MathiasPius
Copy link
Contributor Author

That should do it. Let me know what you think!

Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks great. Thanks a lot for doing this! I see nothing major implementation-wise here i disagree with. Some minor nits inlined (that are largely ignoreable).

One point on maintainability though; tests for this would be helpful. I also think this is self-contained enough that it could live inside a derive.rs module that can be tested therein. That way the only part from this that really needs to live inside of main.rs is the loop over self.derive that decides whether s gets derives (and even that can possibly also be parametrised inside the derive module by passing the s by ref in there, but that's just a stray thought).

LMK what you think.

src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
Co-authored-by: Eirik A <sszynrae@gmail.com>
Signed-off-by: Mathias Pius <contact@pius.io>
@MathiasPius
Copy link
Contributor Author

I think this looks great. Thanks a lot for doing this! I see nothing major implementation-wise here i disagree with. Some minor nits inlined (that are largely ignoreable).

One point on maintainability though; tests for this would be helpful. I also think this is self-contained enough that it could live inside a derive.rs module that can be tested therein. That way the only part from this that really needs to live inside of main.rs is the loop over self.derive that decides whether s gets derives (and even that can possibly also be parametrised inside the derive module by passing the s by ref in there, but that's just a stray thought).

LMK what you think.

Putting it in its own module makes a lot of sense, I avoided doing it because the rest of the project seemed to favor localized code over modularization. I'll try and extract it and see how testability can be written into it.

Signed-off-by: Mathias Pius <contact@pius.io>
Fix trait derivation for structs (proving the need for tests)

Re-add @clux's vec/clone refactor to reduce allocation

Signed-off-by: Mathias Pius <contact@pius.io>
Signed-off-by: Mathias Pius <contact@pius.io>
Signed-off-by: Mathias Pius <contact@pius.io>
Signed-off-by: Mathias Pius <contact@pius.io>
Signed-off-by: Mathias Pius <contact@pius.io>
@MathiasPius
Copy link
Contributor Author

I think that should be it!

Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very clean and separated now. Thanks a lot. I'll leave it here for a day just in case in case anyone else likes to chime in (also left an optional nit) but this is very simple to reason with now and happy to just send this out as is.

src/derive.rs Outdated Show resolved Hide resolved
src/derive.rs Outdated Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
MathiasPius and others added 2 commits May 1, 2024 00:59
Co-authored-by: Eirik A <sszynrae@gmail.com>
Signed-off-by: Mathias Pius <contact@pius.io>
Signed-off-by: Mathias Pius <contact@pius.io>
Signed-off-by: Mathias Pius <contact@pius.io>
Signed-off-by: Mathias Pius <contact@pius.io>
@clux clux changed the title Make --derive more flexible Make --derive more flexible May 1, 2024
@clux clux merged commit 5b00b1a into kube-rs:main May 1, 2024
5 checks passed
@MathiasPius MathiasPius deleted the flexible-derive branch May 1, 2024 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should enums always implement PartialEq, Eq, PartialOrd, Ord?
2 participants