-
Notifications
You must be signed in to change notification settings - Fork 277
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 ->
method registration more flexible and type-safe
#6455
Conversation
✅ Docs Preview ReadyNo new or changed pages found. |
CI performance tests
|
apollo-federation/src/sources/connect/json_selection/methods.rs
Outdated
Show resolved
Hide resolved
8f4c23d
to
62775ad
Compare
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.
Very excited for the enum!!! My only suggestion is that maybe strum
would make to/from name conversions easier? But also maybe not...
// This Deref implementation allows us to call .apply(...) directly on the | ||
// ArrowMethod enum. | ||
impl std::ops::Deref for ArrowMethod { | ||
type Target = dyn ArrowMethodImpl; |
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.
Oh nice, don't think I've ever done a dyn
Deref
before, good to know that's possible.
// As this case suggests, we can't necessarily provide a name() | ||
// method for ArrowMethod (the opposite of lookup), because method | ||
// implementations can be used under multiple names. | ||
"matchIf" | "match_if" => Some(Self::MatchIf), |
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.
We could make the design decision not to have multiple names, though. Seems like we should standardize on camelCase and not snake_case.
That would also mean we could use strum
to impl both FromStr
and Display
for all the methods without all the boilerplate.
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 guess strum
would make it harder to do the public/future split. Might end up having to add a #[cfg(test)]
to every future method...
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 open to ideas, though not previously familiar with strum
.
I'd be ok with sticking to one method name/style, but I think it's worthwhile to keep passing method_name
as an argument into the method apply
/shape
implementations, so there's only one spot in the code we need to change if we want to rename a method (compared to each method implicitly "knowing" its own name and encoding it into error messages, for example).
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.
For sure. I thought about suggesting that the macro could inject the name, but I think we still want location data down in the implementation so it wouldn't save us much.
Previously,
->
methods (#5762) were implemented as functions and registered in alazy_static!
map for dynamic lookup at runtime.This PR refactors the system for registering methods, so there is now an
ArrowMethod
enum with a variant for each known method, and each method is now represented by a struct type that has an::apply
method, whose signature resembles the previous function implementations.In the future, we can add other methods to these structs, so
->
methods can programmatically define their interactions with the output shape of the JSONSelection, for example.