NEW @W-17042397@ Detail output now has multilocation support #1673
Conversation
| message: This is a message | ||
| locations: | ||
| __PATH_TO_FILE_A__:20:1 | ||
| **__PATH_TO_FILE_Z__:2:1 This is a comment at Location 2** |
There was a problem hiding this comment.
What does ** do? Is that just to mark it as primary?
You'll want to ask Marcelino about this one I think to see if he thinks this is a good way to display things.
I'm not sure how I feel about this. Also it may get in the way with the ability to click what turns into a link in vscode and intellij when folks run sf code-analyzer run from the terminal inside of their IDE. It is really a useful feature that /some/file.txt:20:1 is clickable. (This again is one of the reasons why I think we should have detail view as the default view). But if ** is attached ... not sure if it'll recognize the file:line:col pattern.
Maybe instead, we could have:
__PATH_TO_FILE_A__:20:1
> __PATH_TO_FILE_Z__:2:1 This is a comment at Location 2
__PATH_TO_FILE_A__:1:1 This is a comment at Location 3
where the we replace the indent with > is inside of the indent here and it points to the primary without shifting it to the right and while leaving a space before it still.
There was a problem hiding this comment.
Will do. Thanks.
src/lib/viewers/ResultsViewer.ts
Outdated
| function stringifyLocations(codeLocations: CodeLocation[], primaryIndex: number): string[] { | ||
| const locationStrings: string[] = []; | ||
|
|
||
| codeLocations.forEach((loc, idx) => { | ||
| const commentPortion: string = loc.getComment() ? ` ${loc.getComment()}` : ''; | ||
| const rawLocationString: string = `${loc.getFile()}:${loc.getStartLine()}:${loc.getStartColumn()}${commentPortion}`; | ||
| locationStrings.push(idx === primaryIndex ? `**${rawLocationString}**` : rawLocationString); | ||
| }); | ||
|
|
||
| return locationStrings; | ||
| } |
There was a problem hiding this comment.
There is an false assumption here that if the locations length is 1 then there is no comment. This may not be the case with flowtest or with other engines in the future.
So can we instead have on line 83
location: stringifyLocation(primaryLocation)
and then here we would have something like:
function stringifyLocations(codeLocations: CodeLocation[], primaryIndex:number): string[] {
codeLocations.map(stringifyLocations);
codeLocations[primaryIndex] = `**${codeLocations[primaryIndex]}**`;
}
function stringifyLocation(loc: CodeLocation): string {
const commentPortion: string = loc.getComment() ? ` ${loc.getComment()}` : '';
return `${loc.getFile()}:${loc.getStartLine()}:${loc.getStartColumn()}${commentPortion}`;
}
There was a problem hiding this comment.
Counterpoint: If there's only one location, then any comment that location might have is likely to be redundant with the top-level violation message. I'm trying to imagine a reason that they'd contain completely different information and nothing's coming to mind.
There was a problem hiding this comment.
I see your point... so the engine authors really shouldn't be doing this. But in the event they do - and they may have a legitimate reason for it... do we really want to prevent their comment from being seen? So best to code the clients to the core api and what it can can't do instead of assuming the current use cases will be the same as the future use cases.
src/lib/viewers/ResultsViewer.ts
Outdated
| codeLocations.forEach((loc, idx) => { | ||
| const commentPortion: string = loc.getComment() ? ` ${loc.getComment()}` : ''; | ||
| const locationString: string = `${loc.getFile()}:${loc.getStartLine()}:${loc.getStartColumn()}${commentPortion}`; | ||
| const mainPortion: string = `${primaryIndex != null && primaryIndex === idx ? '(main) ' : ''}`; |
There was a problem hiding this comment.
Is there a reason you put this all in backticks?
Why not just do:
const mainPortion: string = primaryIndex != null && primaryIndex === idx ? '(main) ' : '';
There was a problem hiding this comment.
No, you're right, that's the better way to do it.
No description provided.