-
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
[JSONSelection] Remove &mut errors: IndexSet<ApplyToError>
from apply_to_path
method signatures
#6003
[JSONSelection] Remove &mut errors: IndexSet<ApplyToError>
from apply_to_path
method signatures
#6003
Conversation
CI performance tests
|
064973b
to
20beea1
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.
The more functional style feels much nicer to me, thanks! I think it will be a more efficient in the error case to switch to passing owned values instead of references which we reallocate on. Might shake up too many other things, though, so if it ends up being a mess, we can stick with this.
// Rust doesn't allow implementing methods directly on tuples like | ||
// (Option<JSON>, Vec<ApplyToError>), so we define a trait to provide the | ||
// methods we need, and implement the trait for the tuple in question. | ||
pub(super) trait ApplyToResultMethods { |
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.
Nice trick, I always forget that I can hack in methods like this
apollo-federation/src/sources/connect/json_selection/apply_to.rs
Outdated
Show resolved
Hide resolved
apollo-federation/src/sources/connect/json_selection/apply_to.rs
Outdated
Show resolved
Hide resolved
apollo-federation/src/sources/connect/json_selection/helpers.rs
Outdated
Show resolved
Hide resolved
Following up on this suggestion from @nicholascioli: #5762 (comment)
20beea1
to
1a44860
Compare
Following up on this suggestion from @nicholascioli: #5762 (comment)
The
apply_to_path
method used to have more than one mutable parameter (input_path
as well aserrors
), but now thatinput_path
is immutable, I agree it's cleaner to go all the way and avoid any mutable parameters, soapply_to_path
can be a pure function.One big benefit of this refactoring is that each
apply_to_path
method now returns just the errors resulting from the subtree that it handled, whereas the previous&mut errors: IndexSet<ApplyToError>
strategy made it difficult to know which errors came from whichapply_to_path
method call. This will help if we ever decide to memoize the results ofapply_to_path
methods, or if we want to exposeApplyToError
s for processing by->
methods (imagine some sort of->catch
handler that captures/handles errors from further down itsPathList
chain).As discussed in #5987 (comment), there might not be much point in deduplicating errors now, since their
path
andrange
information effectively makes them all unique, but if we decide to stop the deduplication that's now an easy change withinJSONSelection::apply_with_vars
.These changes should be entirely an implementation detail. As evidence for this claim, I did not have to change any tests.