fix: plugin warnings not showing warning.loc #3824
Conversation
Plugin warnings without warning.id but with warning.loc were not emitting loc information loc information emitting is fundamental for user experience (to locate the source of the error/warning)
There was a problem hiding this comment.
This looks like a useful improvement to me. However, some work would be needed to get this ready for release, see my comment.
We would also need tests in order for this to be merged. And there are two different cases to be considered, one including loc.file and one where it is missing. These could be combined into a single test that emits two warnings, though. You could take for instance test/cli/samples/custom-frame as an inspiration.
Please let me know if you would be willing to go through with this.
cli/run/batchWarnings.ts
Outdated
| @@ -196,6 +196,9 @@ const deferredHandlers: { | |||
| } | |||
| stderr(bold(loc)); | |||
| } | |||
| else if (warning.loc) { | |||
| info(yellow(`${rollup.relativeId(warning.loc.file)}:${warning.loc.line}:${warning.loc.column}`)+' error '+message); | |||
| } | |||
There was a problem hiding this comment.
I'm. not happy about this being the only non-title being formatted in yellow, couldn't we stick with the formatting used in the previous case? Also why do you want to repeat the error message that has just been written in the title? Moreover, file is optional in .loc, and this only makes sense if file is actually present otherwise you would not know what the location means.
Also this repeats the way that locations are formatted from the previous section, but in a slightly inconsistent way with regard to spacing. Here would be one suggestion to replace everything starting from line 192 with less duplication:
const id = warning.id || warning.loc?.file;
if (id) {
let loc = relativeId(id);
if (warning.loc) {
loc += `: (${warning.loc.line}:${warning.loc.column})`;
}
stderr(bold(loc));
}There was a problem hiding this comment.
Lukas, I changed the code as you suggested.
I won't be able to add tests.
Codecov Report
@@ Coverage Diff @@
## master #3824 +/- ##
=======================================
Coverage 97.06% 97.06%
=======================================
Files 184 184
Lines 6502 6503 +1
Branches 1883 1884 +1
=======================================
+ Hits 6311 6312 +1
Misses 101 101
Partials 90 90
Continue to review full report at Codecov.
|
|
I can push a test to your branch, but you would need to "allow edits by maintainer", which should be a checkbox somewhere to the bottom right of your PR: |
|
done. |
Plugin warnings without warning.id but with warning.loc were not emitting loc information
loc information emitting is fundamental for user experience (to locate the source of the error/warning)
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description
Plugin warnings without
warning.idbut withwarning.locwere not emittingwarning.locinformationwarning.locinformation emitting is fundamental for user experience (to locate the source of the error/warning)added 3 lines, to emit the
warning.locinfo if present in a format amicable to IDEs problem matchers: file:line:col