Conversation
this also improves performance and simplifies pretty-printing
see apache#1850 all the tests are skipped at the moment, we will enable them as we implement more pretty printing
- Refactor DisplayCommaSeparated to accept any iterable type. - Update indented_list to work with iterators. - Improve formatting in the Insert display implementation
| fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
| let mut first = true; | ||
| for t in self.0 { | ||
| for t in self.0.clone() { |
There was a problem hiding this comment.
was the clone needed here, we seem to only call fmt on t?
There was a problem hiding this comment.
Yes, we are never actually cloning anything expensive here, but we need to add the Clone generic bound in order to be able to use the IntoIterator trait.
This .clone() concretely always clones
- either just a reference (to a slice)
- or an iterator object (but not the underlying structure being iterated on)
There was a problem hiding this comment.
@iffyio , I can roll it back to the non-generic version, and just implement comma separation manually in the Values Display impl, if you want.
There was a problem hiding this comment.
Yeah I think it might be better to make it explicit, with the new impl seems like it wouldn't be obvious if we were to go on to later clone something expensive accidentally
There was a problem hiding this comment.
@iffyio : I reverted the generic indented_list implementation and reworked the Values Display implementation not to use it.
lovasoa
force-pushed
the
pretty-print-improvements
branch
2 times, most recently
from
da1499a to
064132c
Compare
lovasoa
force-pushed
the
pretty-print-improvements
branch
from
064132c to
75ec6b9
Compare
|
woohoo! |
3 participants
this is a first PR for #1850
mainly implementing pretty printing for insert update and delete statements