misc(compare-runs): category scores, url filter, sort abs.val #10526
Conversation
| const lhr = JSON.parse(lhrJson); | ||
| const lhr = lhrJson && JSON.parse(lhrJson); | ||
| if (!lhr || !lhr.audits) { | ||
| console.warn(`could not parse ${lhrPath}`); |
There was a problem hiding this comment.
This isn't strictly true, if !lhr was false, but !lhr.audits was true, this just implies that there were no audits parsed. It still may have been parsed fine, might just not have audits. This error might not be specific enough to care the diff, but it is misleading.
| const metrics = lhr.audits.metrics ? | ||
| if (argv.urlFilter && !lhr.requestedUrl.includes(argv.urlFilter)) continue; | ||
|
|
||
| const metrics = lhr.audits.metrics && lhr.audits.metrics.details ? |
There was a problem hiding this comment.
This is starting to get out of hand. && and a ternary in 1 assignment 😟
Am I too go-thonic to say
const metrics = {}
if (lhr.audits.metrics && lhr.audits.metrics.details) {
metrics = /** @type {!LH.Audit.Details.Table} */ (lhr.audits.metrics.details).items[0];
}
Honestly curious if it's more readable one way or the other. I think once a ternary becomes complex at all it's time to separate it out.
There was a problem hiding this comment.
if only lhr.audits.metrics?.details ...
Am I too go-thonic to say
i think it's gooey :P I would prefer it if the ternary were more complex, but as it is it's just a nullability check.
| categories: Object.values(lhr.categories).reduce((acc, cur) => { | ||
| acc.push([cur.id, (cur.score || 0) * 100]); | ||
| return acc; | ||
| }, /** @type {Array<[string, number]>} */ ([])), |
There was a problem hiding this comment.
Seems weird to output this as an array of strings and numbers? And not as a map of strings:numbers? Any reason to output like this?
There was a problem hiding this comment.
The allEntries object expects this format. Did you see how it's used in the following loop, L241?
The format comes naturally from Object.entries(metrics) -> Array<[kind: string, value: number]>
connorjclark
changed the title
More stuff for compare runs script (related: #10519)