kube-rs / kopium

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

Should enums always implement PartialEq, Eq, PartialOrd, Ord? #231

Closed MathiasPius closed 6 months ago

MathiasPius commented 6 months ago

Are there any examples where it doesn't make sense to derive these traits for an enum?

In my case I need to compare enums generated by Kopium, but specifying the derives using --derive flag does not work because of externally defined structures like Condition.


I've implemented my take on this here https://github.com/MathiasPius/kopium

Dav1dde commented 6 months ago

Could be tricky if there are enums containing floats (or other types which do not implement the mentioned traits), maybe just derive for "simple" enums (containing no data).

MathiasPius commented 6 months ago

Really good point! I've constrained my implementation to enums where all members satisfy the predicate of having an empty type_

clux commented 6 months ago

you could also technically have resources with non-trivial relations implemented out-of-band that are impossible to represent in an openapi schema. a simple enum example could be a state machine representation where each variant represents a phase, and an ordering is used to represent how far along the pipeline the phase is.

MathiasPius commented 6 months ago

you could also technically have resources with non-trivial relations implemented out-of-band that are impossible to represent in an openapi schema.

That's true, but I think that would fall outside the scope of Kopium right? If we assume the PartialOrd/Ord derivation is constrained to simple enums with no fields, then I struggle to think of an enum whose internal ordering is so complex that it cannot be represented in an OpenAPI schema.

a simple enum example could be a state machine representation where each variant represents a phase, and an ordering is used to represent how far along the pipeline the phase is.

I think this is a really good counterpoint! I would perhaps argue that the ordering should then also be represented in the OpenAPI schema, whereby the derivation would automatically derive the correct ordering.

So to summarize:

  1. It should always be safe to derive PartialEq and Eq for simple enums.
  2. There might be cases where users want to PartialOrd and Ord themselves, or omit it altogether to avoid confusion with other non-Ord ordering mechanics they implement out of band.

Could PartialOrd/Ord derivation on simple enums be gated behind a flag for example?

clux commented 6 months ago

Could PartialOrd/Ord derivation on simple enums be gated behind a flag for example?

Yeah, if it's helpful that could be done. In the interest in avoiding a combinatorial explosion of opt-ins/opt-outs, maybe something general like --simple-enum-derive=PartialEq,PartialOrd,Ord,Eq as this feels pretty niche, but a very specific flag like that is easy enough to isolate.

MathiasPius commented 6 months ago

I was considering perhaps modifying --derive to optionally allow specifying the name of the object you would like the rule to apply to, defaulting to "all" to maintain backwards compatibility.

So for example:

# Standard usage, applies to all generated structures.
$ kopium --derive PartialEq issuers.cert-manager.io

# Target specific object (just an enum in this case)
$ kopium --derive IssuerAcmeSolversDns01CnameStrategy=PartialEq issuers.cert-manager.io

# Target all structs
$ kopium --derive @struct=PartialEq issuers.cert-manager.io

# Target all enums
$ kopium --derive @enum=PartialEq issuers.cert-manager.io

# Target simple enums
$ kopium --derive @enum:simple=PartialEq issuers.cert-manager.io

--simple-enum-derive would be a lot easier to implement, of course, but the other method is a bit more flexible.

I'd be happy to implement either solution, and of course the exact syntax is very much up for discussion.

clux commented 6 months ago

Ah, I like this a lot more than my suggestion! Syntax makes sense, it's pretty unobtrusive in that you opt-in to as much complexity as you need by tweaking the same --derive flag(s). I imagine this will need some smarter matching mechanism in the output module to check if the current struct matches an element in the vector of derive rules, but that also sounds like it could be isolated in a testable place.

Would be very happy to have this!