[CHORE] Add derive_more to get rid of manual Display impls
#2794
Conversation
CodSpeed Performance ReportMerging #2794 will not alter performanceComparing Summary
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2794 +/- ##
=======================================
Coverage ? 63.31%
=======================================
Files ? 1008
Lines ? 114154
Branches ? 0
=======================================
Hits ? 72282
Misses ? 41872
Partials ? 0
|
- updated some breaking API changes
|
@raunakab you should wait on a review before merging some of these bigger PRs. I was in the middle of one for this PR. |
|
Sorry about that! Guess I jumped the gun here. |
| func: &FunctionExpr, | ||
| inputs: &[ExprRef], | ||
| ) -> std::result::Result<String, std::fmt::Error> { | ||
| let mut f = String::default(); |
There was a problem hiding this comment.
can we write it out directly to std::fmt::Writer?
There was a problem hiding this comment.
How so? I don't think std::fmt::Writer exists. Did you mean std::fmt::Write? That's a trait, however.
There was a problem hiding this comment.
Whoops I meant Formatter
I was wondering if instead of writing to a string (heap allocation) which then gets written to the Formatter, if we can write directly to the formatter
For example, when we impl the Trait, we can do that.
impl Display for X {
fn fmt(&self, f: &mut Formatter) -> Result {}
}
There was a problem hiding this comment.
Hm. Not sure how to get access to the Formatter instance that the fmt function gets as its second argument. The docs say not to construct one yourself.
There was a problem hiding this comment.
Ya so it looks like we can't directly construct a Formatter instance, nor are we able to access the Formatter through the #[display("{}", ...)] macro. I was trying to do something like #[display(f, "{}", ...)], but that wasn't working. Looks like derive_more doesn't want its users to access the underlying formatter directly.
There was a problem hiding this comment.
If we want access to the formatter, we'd need to revert back to the manual implementation of Display for Expr.
| left: &ExprRef, | ||
| right: &ExprRef, | ||
| ) -> std::result::Result<String, std::fmt::Error> { | ||
| let mut f = String::default(); |
| #[serde(transparent)] | ||
| #[display("{}\n", make_schema_vertical_table( |
There was a problem hiding this comment.
I think the former is actually more readable than this!
There was a problem hiding this comment.
Would you want the manual impl Display for ... to be used here? Or create a new wrapper helper function which would do something similar?
|
@raunakab Just published my review, let's fix in a follow on? |
Yup! Will open up a follow-up PR soon. |
# Overview Follow up PR for #2794.
Overview
derive_more::Displayfor a couple structs to remove some boilerplate.Note
This has the potential of increasing build times due to increasing the amount of codegen
rustcwill have to do internally. I can try and put up some build time metrics in order to compare the differences in build times.