Group errors by line number and show count #7299
Conversation
webapp/src/errorList.tsx
Outdated
|
|
||
| let errorCount = 0; | ||
| (errors || []).forEach(e => errorCount += e.count); |
There was a problem hiding this comment.
nit: (errors || []).reduce((e, count) => e.count + count)
There was a problem hiding this comment.
nit:
(errors || []).reduce((e, count) => e.count + count)
Delayed grouping made this unnecessary. Reverted my changes in this method.
webapp/src/errorList.tsx
Outdated
| } | ||
|
|
||
| function groupErrors(errors: pxtc.KsDiagnostic[]) { | ||
| const grouped: { [key: string]: GroupedError} = {}; |
There was a problem hiding this comment.
nit - we have a type def for maps: Map<GroupedError>
There was a problem hiding this comment.
nit - we have a type def for maps:
Map<GroupedError>
Oh yes, the Map type!
There was a problem hiding this comment.
{ [key: string]: GroupedError} is standard TS syntax and we should prefer that IMO. I'd like to bring this up at the next code review day
webapp/src/errorList.tsx
Outdated
| @@ -94,7 +101,7 @@ export class ErrorList extends React.Component<ErrorListProps, ErrorListState> { | |||
|
|
|||
| onErrorsChanged(errors: pxtc.KsDiagnostic[]) { | |||
| this.setState({ | |||
| errors, | |||
| errors: groupErrors(errors), | |||
There was a problem hiding this comment.
IMO, we should lazily group errors if the error list is visible. In most cases, it is collapsed and this computation is not required.
There was a problem hiding this comment.
IMO, we should lazily group errors if the error list is visible. In most cases, it is collapsed and this computation is not required.
Agreed that's better. Updated.
| } | ||
| } | ||
| let sorted: GroupedError[] = []; | ||
| grouped.forEach(value => sorted.push(value)); |
There was a problem hiding this comment.
sorted = [...grouped].sort((a, b) => a.index - b.index)
| } | ||
|
|
||
| function groupErrors(errors: pxtc.KsDiagnostic[]) { | ||
| const grouped = new Map<string, GroupedError>(); |
There was a problem hiding this comment.
I'm curious, what are the advantages of using a map type over vanilla objects?
There was a problem hiding this comment.
we've been using consistently the Map type accross the entire PXT codebase; so to stay consistent, keep using it. Also, don't have to lookup the TS index syntax every time.
webapp/tsconfig.json
Outdated
| @@ -16,6 +16,7 @@ | |||
| "emitDecoratorMetadata": true, | |||
| "declaration": false, | |||
| "preserveConstEnums": true, | |||
| "downlevelIteration": true, | |||
There was a problem hiding this comment.
@pelikhan use of spread operator on map values required the addition of this compiler option (due to targeting es5). Any issue?
There was a problem hiding this comment.
I'll revert the change to avoid risk. Curious what the language gurus think though.
There was a problem hiding this comment.
I think it should be fine. There's lots of things we do in TS that we cannot do in STS (like async).
Duplicate compiler errors are grouped, and a count bubble is shown (if count > 1).
Before:
After:
(screenshots not taken from the same project, so total error count is different)
Fixes: microsoft/pxt-arcade#2105