-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
bevy_reflect: enum_utility
cleanup
#13424
bevy_reflect: enum_utility
cleanup
#13424
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some small comments; overall the new structure is clearer to me!
/// 1. Use `field_accessor` to access the field as an `Option<&dyn Reflect>`. | ||
/// 2. Use `field_unwrapper` to unwrap the field into a `&dyn Reflect`. | ||
/// 3. Use `field_constructor` to construct the field into a concrete value. | ||
struct EnumVariantOutputDataBuilder<'a> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this design with field_accessor
, field_unwrapper
, field_constructor
is the clearest.
Maybe just having a BaseEnumVariantOutputDataBuilder
and then two different structs sEnumVariantOutputDataBuilderForFromReflect
and EnumVariantOutputDataBuilderForTryApply
that share the underlying BaseEnum...
but have different implementations for the unwrapper
and constructor
would be easier to understand?
I don't think it's a blocker, because you're the one primarily maintaining this code so it's already an improvement if it's clearer to you. I think the new structure is overall much clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just having a
BaseEnumVariantOutputDataBuilder
and then two different structs sEnumVariantOutputDataBuilderForFromReflect
andEnumVariantOutputDataBuilderForTryApply
that share the underlyingBaseEnum...
but have different implementations for theunwrapper
andconstructor
would be easier to understand?
Hm, could you give an example? I'm probably misunderstanding, but this almost sounds like what I have now with the base builder type and the two pub(crate)
generator functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm I guess you're right.
I was thinking of something with generics
trait EnumTokenBuilder {
fn field_accessor(&Ident, VariantField) -> TokenStream;
fn ...
}
just to avoid the Box<dyn Fn>
and having to build the struct without those functions existing.
I guess I'm just not a fan of the builder pattern in this instance, especially if the fields are not optional. A new contributor might not understand why the new
function doesn't just require those fields.
It's mostly a matter of taste, though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah tbh I don't love it either. Alternatively, I could scrap both new
and the with_***
and just have it all defined like:
let builder = EnumVariantOutputDataBuilder {
this,
reflect_enum,
field_accessor: Box::new(/* ... */),
// ...
};
builder.build()
It just means that any fields with a potential default value will need to be defined each time.
The trait method you propose is nice too and is probably a bit Rustier. I might look into that actually. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the code to use traits instead. I think it's a lot cleaner so thank you for the suggestion!
Let me know if you have any further thoughts on it (or if you even prefer the old version haha)!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it!
I also think it's cleaner with the traits rather than the boxed functions :)
@MrGVSV once this has merge conflicts resolved I'll merge this. |
3a1c007
to
f46cd26
Compare
Objective
The
enum_utility
module contains theget_variant_constructors
function, which is used to generate token streams for constructing enums. It's used for theFromReflect::from_reflect
implementation and theReflect::try_apply
implementation.Due to the complexity of enums, this function is understandably a little messy and difficult to extend.
Solution
Clean up the
enum_utility
module.Now "clean" is a bit subjective. I believe my solution is "cleaner" in that the logic to generate the tokens are strictly coupled with the intended usage. Because of this,
try_apply
is also no longer strictly coupled withfrom_reflect
.This makes it easier to extend with new functionality, which is something I'm doing in a future unrelated PR that I have based off this one.
Testing
There shouldn't be any testing required other than ensuring that the project still builds and that CI passes.