[crater] Show per-table deltas in diff detail view #938
Conversation
cmyr
force-pushed
the
crater-diff-detail-decoration
branch
from
398d4c9
to
3848727
Compare
fontc_crater/src/ci/html.rs
Outdated
| fn make_delta_decoration<T: PartialOrd + Copy + Sub<Output = T> + Display + Default>( | ||
| current: T, | ||
| prev: Option<T>, | ||
| more_is_better: bool, |
There was a problem hiding this comment.
Suggest enum instead of bool, the callsites are a bit opaque. A trait might be nice? - ratio.delta_decoration(prev, Better::More)
There was a problem hiding this comment.
added the enum, trait is definitely an option but would rather just get this merged than noodle on it much more. :)
fontc_crater/src/ci/html.rs
Outdated
| None | Some(std::cmp::Ordering::Greater) if more_is_better => { | ||
| html! { span.better { (diff) } } | ||
| } | ||
| Some(std::cmp::Ordering::Less) if !more_is_better => html! { span.better { (diff) } }, |
There was a problem hiding this comment.
shouldn't this be |'d onto the case above?
There was a problem hiding this comment.
there can only be a single if clause on a match arm, and it applies after the match (it is not associated with a specific match item, see https://doc.rust-lang.org/1.80.1/reference/expressions/match-expr.html#match-expressions)
There was a problem hiding this comment.
TIL. Or re-learned I suppose since I'm sure I did read that at some point.
That is, in the detail view where it shows the per-table diff percentages for a given font, if there have been any changes from the previous run display the delta at the per-table level, so we can better understand what changed.
cmyr
force-pushed
the
crater-diff-detail-decoration
branch
from
3848727
to
8175de3
Compare
That is, in the detail view where it shows the per-table diff percentages for a given font, if there have been any changes from the previous run display the delta at the per-table level, so we can better understand what changed.
You can see how this looks at https://googlefonts.github.io/fontc_crater/