search

apollographql / federation-next

Moved to the Router repository
https://github.com/apollographql/router/tree/dev/apollo-federation
Other
19 stars 1 forks source link

FED-47: Finish porting `compute_best_plan_from_closed_branches` #277

Closed duckki closed 4 months ago

duckki commented 4 months ago

~Mostly finished, but still in progress. I wanted to get some feedback in a few areas.~

This PR is now ready for review.

OpPathTree::merge(&self, ...) -> Self

~I needed OpPathTree::merge(&self, ...) -> Self, but merge name is taken. We already have PathTree::merge(self: &Arc<Self>, ...) -> Arc<Self>. Probably, we can refactor the common part out. (@SimonSapin What do you think? Would there be a better way?) But, how would you like to name the new method? I tentatively chose merge_raw.~

Update: I decided to use the existing PathTree::merge method and it worked.

impl Clone for FetchDependencyGraph

~I added a simple cloning, but it's probably wrong, since the JS version looks a lot more complicated.~

Update: Sachin confirmed the derived Clone implementation would suffice.

Debug logging

~JS code has debug.log(...) lines. How do we want to port those?~

Update: I left the logging code, but commented it out. We can decide what to do after rover integration.

duckki commented 4 months ago

@lrlna Thanks for the review. I think I addressed most of your comments. But, I kept fmt_indented and the trait for generate_all_plans_and_find_best argument. I hope that's ok with you. I'll go ahead and merge.