core: remove last debugString references #5856
Conversation
patrickhulce
requested review from
brendankenny,
ebidel and
paulirish
as code owners
| @@ -37,21 +37,21 @@ const ALLOWED_ORIENTATION_VALUES = [ | |||
| */ | |||
| function parseString(raw, trim) { | |||
| let value; | |||
| let debugString; | |||
There was a problem hiding this comment.
seems like we want the manifest-parser still returning something like debugString? explanation seems too generic for what is always indicating an error/warning. Gatherers that use it would still only use the standard thing
There was a problem hiding this comment.
Originally in manifest-parser it was warning, which got switched to debugString, which was then spread throughout Lighthouse cause why not, all in #102 :)
There was a problem hiding this comment.
explanation seems too generic for what is always indicating an error/warning.
sounds like you're upset with our decision to use explanation period :)
I'm not 100% sure what you're advocating for, is it for this PR to just remove the TODO and fix the tests then since we don't want to pursue it?
There was a problem hiding this comment.
sounds like you're upset with our decision to use
explanationperiod :)
haha, well, explanation on an audit is a little confusing in isolation (we really should add a jsdoc description to it in audit.d.ts :), but it comes in the context of a failing audit.
In this case the only indication that there was an issue is the existence of the string. I meant just for manifest-parser.js, it's not in the gatherer or audit pipelines, it's just a lib file, so it seems fine to have a more descriptive property name specific to that file.
There was a problem hiding this comment.
looked and within manifest-parser, i'd say that, semantically, all these properties are really warnings. Except for the errorMessage at the end about invalid JSON
So i'd be in favor of changing the manifestparser explanation->warning for all items and errorMesssage on the toplevel. and also tweaking manifest-values.js to use these terms as well.
There was a problem hiding this comment.
@brendankenny are you on board with warning as well? I think it reads rather well now (aside from the fact that it's warning = 'ERROR: 😆)
There was a problem hiding this comment.
are you on board with warning as well?
yes, sounds good to me
(aside from the fact that it's warning = 'ERROR: 😆)
lol yes, that's terrible
| @@ -425,7 +425,7 @@ function parse(string, manifestUrl, documentUrl) { | |||
| return { | |||
| raw: string, | |||
| value: undefined, | |||
| debugString: 'ERROR: file isn\'t valid JSON: ' + e, | |||
| warning: 'ERROR: file isn\'t valid JSON: ' + e, | |||
There was a problem hiding this comment.
I think @paulirish wanted this to be errorMessage, as this was the one true place it can fail (everywhere else it falls back to a default)
There was a problem hiding this comment.
(it will be interesting to see if you change it just here (and errorMessage: undefined below) if tsc will correctly pick up all the places it needs to be adjusted automatically since the artifact is dynamically typed from the inferred return value of this function...)
There was a problem hiding this comment.
Oh meant to reply to this, this was an intentional feigned ignorance about that part of the comment ;)
I don't think manifest-parser should to try to name its properties based on how one audit will pass/fail because of its value. From the perspective of manifest-parser all of these strings are messages explaining that something was abnormal about the raw value (there are other cases that we ignore the string and provide a default where it's a warning).
Since we're taking the stance that these are all warnings and not total failures, seems like we should lean in to it :)
There was a problem hiding this comment.
on the one hand.. this field represents why the manifest parser was unable to parse.
but on the other hand.. i dont really care. 😑 i'm fine with warning if you'd prefer it.
There was a problem hiding this comment.
lol, I'd prefer it given the agreement we have around everything else in there labelled ERROR: being a warning :)
sg!
There was a problem hiding this comment.
this field represents why the manifest parser was unable to parse.
I also agree with this. It's the only real failure for a manifest. Everything else is a warning that something was malformed but the browser will fall back to a default, so the manifest will still work.
But I also don't care that much :) it's just (minor) ergonomics of the result, not correctness.
| @@ -19,7 +19,7 @@ const BOM_FIRSTCHAR = 65279; | |||
| class Manifest extends Gatherer { | |||
| /** | |||
| * Returns the parsed manifest or null if the page had no manifest. If the manifest | |||
| * was unparseable as JSON, manifest.value will be undefined and manifest.debugString | |||
| * was unparseable as JSON, manifest.value will be undefined and manifest.explanation | |||
|
Long live |
Remaining cleanup from the debugString change, updates the gatherers to return explanation instead of debugString to align with our output in LHR.