-
Notifications
You must be signed in to change notification settings - Fork 1
FED-47: Finish porting compute_best_plan_from_closed_branches
#277
Changes from all commits
d132e6c
2d4fb07
d92e45f
3c90384
e1163d4
c861788
1d6d51d
72ba969
85718f9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
use std::fmt; | ||
|
||
pub(crate) struct State<'fmt, 'fmt2> { | ||
indent_level: usize, | ||
output: &'fmt mut fmt::Formatter<'fmt2>, | ||
} | ||
|
||
impl<'a, 'b> State<'a, 'b> { | ||
pub(crate) fn new(output: &'a mut fmt::Formatter<'b>) -> State<'a, 'b> { | ||
Self { | ||
indent_level: 0, | ||
output, | ||
} | ||
} | ||
|
||
pub(crate) fn indent_level(&self) -> usize { | ||
self.indent_level | ||
} | ||
|
||
pub(crate) fn write<T: fmt::Display>(&mut self, value: T) -> fmt::Result { | ||
write!(self.output, "{}", value) | ||
} | ||
|
||
pub(crate) fn write_fmt(&mut self, args: fmt::Arguments<'_>) -> fmt::Result { | ||
self.output.write_fmt(args) | ||
} | ||
|
||
pub(crate) fn new_line(&mut self) -> fmt::Result { | ||
self.write("\n")?; | ||
for _ in 0..self.indent_level { | ||
self.write(" ")? | ||
} | ||
Ok(()) | ||
} | ||
|
||
pub(crate) fn indent_no_new_line(&mut self) { | ||
self.indent_level += 1; | ||
} | ||
|
||
pub(crate) fn indent(&mut self) -> fmt::Result { | ||
self.indent_no_new_line(); | ||
self.new_line() | ||
} | ||
|
||
pub(crate) fn dedent(&mut self) -> fmt::Result { | ||
self.indent_level -= 1; | ||
self.new_line() | ||
} | ||
} | ||
|
||
pub(crate) fn write_indented_lines<T>( | ||
state: &mut State<'_, '_>, | ||
values: &[T], | ||
mut write_line: impl FnMut(&mut State<'_, '_>, &T) -> fmt::Result, | ||
) -> fmt::Result { | ||
if !values.is_empty() { | ||
state.indent_no_new_line(); | ||
for value in values { | ||
state.new_line()?; | ||
write_line(state, value)?; | ||
} | ||
state.dedent()?; | ||
} | ||
Ok(()) | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,4 +1,5 @@ | ||||||||||||||||||||||||||||||||||||||||
use crate::error::FederationError; | ||||||||||||||||||||||||||||||||||||||||
use crate::indented_display::{write_indented_lines, State as IndentedFormatter}; | ||||||||||||||||||||||||||||||||||||||||
use crate::is_leaf_type; | ||||||||||||||||||||||||||||||||||||||||
use crate::link::federation_spec_definition::get_federation_spec_definition_from_subgraph; | ||||||||||||||||||||||||||||||||||||||||
use crate::link::graphql_definition::{ | ||||||||||||||||||||||||||||||||||||||||
|
@@ -477,6 +478,22 @@ impl Display for OpGraphPathContext { | |||||||||||||||||||||||||||||||||||||||
#[derive(Clone)] | ||||||||||||||||||||||||||||||||||||||||
pub(crate) struct SimultaneousPaths(pub(crate) Vec<Arc<OpGraphPath>>); | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
impl SimultaneousPaths { | ||||||||||||||||||||||||||||||||||||||||
pub(crate) fn fmt_indented(&self, f: &mut IndentedFormatter) -> std::fmt::Result { | ||||||||||||||||||||||||||||||||||||||||
match self.0.as_slice() { | ||||||||||||||||||||||||||||||||||||||||
[] => f.write("<no path>"), | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
[first] => f.write_fmt(format_args!("{{ {first} }}")), | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
_ => { | ||||||||||||||||||||||||||||||||||||||||
f.write("{")?; | ||||||||||||||||||||||||||||||||||||||||
write_indented_lines(f, &self.0, |f, elem| f.write(elem))?; | ||||||||||||||||||||||||||||||||||||||||
f.write("}") | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
impl std::fmt::Debug for SimultaneousPaths { | ||||||||||||||||||||||||||||||||||||||||
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { | ||||||||||||||||||||||||||||||||||||||||
f.debug_list() | ||||||||||||||||||||||||||||||||||||||||
|
@@ -485,6 +502,12 @@ impl std::fmt::Debug for SimultaneousPaths { | |||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
impl std::fmt::Display for SimultaneousPaths { | ||||||||||||||||||||||||||||||||||||||||
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { | ||||||||||||||||||||||||||||||||||||||||
self.fmt_indented(&mut IndentedFormatter::new(f)) | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
Comment on lines
+505
to
+510
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd just have the fmt_indented straight up in here, I don't think it needs an extra Impl block. I also feel a bit strange about matching on sliced elements. A nicer pattern would be to do a
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Although As to matching There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with Iryna that we should just move that logic to the display trait as even if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's an anti-pattern in Rust to have formatting done in a non-Display or non-Debug implementations. The Rust implementation should be using those whenever a formatted version of a struct is needed, and not a one-off method. It's an expectation of the ecosystem. |
||||||||||||||||||||||||||||||||||||||||
/// One of the options for an `OpenBranch` (see the documentation of that struct for details). This | ||||||||||||||||||||||||||||||||||||||||
/// includes the simultaneous paths we are traversing for the option, along with metadata about the | ||||||||||||||||||||||||||||||||||||||||
/// traversal. | ||||||||||||||||||||||||||||||||||||||||
|
@@ -693,11 +716,33 @@ enum UnadvanceableReason { | |||||||||||||||||||||||||||||||||||||||
/// set, and the `SimultaneousPaths` ends at the node at which that query is made instead of a node | ||||||||||||||||||||||||||||||||||||||||
/// for the leaf field. The selection set gets copied "as-is" into the `FetchNode`, and also avoids | ||||||||||||||||||||||||||||||||||||||||
/// extra `GraphPath` creation and work during `PathTree` merging. | ||||||||||||||||||||||||||||||||||||||||
#[derive(Debug)] | ||||||||||||||||||||||||||||||||||||||||
pub(crate) struct ClosedPath { | ||||||||||||||||||||||||||||||||||||||||
pub(crate) paths: SimultaneousPaths, | ||||||||||||||||||||||||||||||||||||||||
pub(crate) selection_set: Option<Arc<NormalizedSelectionSet>>, | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
impl ClosedPath { | ||||||||||||||||||||||||||||||||||||||||
pub(crate) fn flatten( | ||||||||||||||||||||||||||||||||||||||||
&self, | ||||||||||||||||||||||||||||||||||||||||
) -> impl Iterator<Item = (&OpGraphPath, Option<&Arc<NormalizedSelectionSet>>)> { | ||||||||||||||||||||||||||||||||||||||||
self.paths | ||||||||||||||||||||||||||||||||||||||||
.0 | ||||||||||||||||||||||||||||||||||||||||
.iter() | ||||||||||||||||||||||||||||||||||||||||
.map(|path| (path.as_ref(), self.selection_set.as_ref())) | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
impl std::fmt::Display for ClosedPath { | ||||||||||||||||||||||||||||||||||||||||
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { | ||||||||||||||||||||||||||||||||||||||||
if let Some(ref selection_set) = self.selection_set { | ||||||||||||||||||||||||||||||||||||||||
write!(f, "{} -> {}", self.paths, selection_set) | ||||||||||||||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||||||||||||||
write!(f, "{}", self.paths) | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
/// A list of the options generated during query planning for a specific "closed branch", which is a | ||||||||||||||||||||||||||||||||||||||||
/// full/closed path in a GraphQL operation (i.e. one that ends in a leaf field). | ||||||||||||||||||||||||||||||||||||||||
pub(crate) struct ClosedBranch(pub(crate) Vec<Arc<ClosedPath>>); | ||||||||||||||||||||||||||||||||||||||||
|
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'd just have this as part of Display impl below (see the following suggestion)