Truncating lineText in references
#51137
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
| @@ -3522,7 +3522,26 @@ namespace ts.server { | |||
| const scriptInfo = Debug.checkDefined(projectService.getScriptInfo(fileName)); | |||
| const span = toProtocolTextSpanWithContext(textSpan, contextSpan, scriptInfo); | |||
| const lineSpan = scriptInfo.lineToTextSpan(span.start.line - 1); | |||
| const lineText = scriptInfo.getSnapshot().getText(lineSpan.start, textSpanEnd(lineSpan)).replace(/\r|\n/g, ""); | |||
| if (lineSpan.length > 1024) { | |||
| var start = (span.start.offset > 500) ? (span.start.offset - 501 + lineSpan.start) : lineSpan.start; | |||
There was a problem hiding this comment.
We never use var because it has weird scoping (e.g. lineText below is visible outside the if block).
| @@ -3522,7 +3522,26 @@ namespace ts.server { | |||
| const scriptInfo = Debug.checkDefined(projectService.getScriptInfo(fileName)); | |||
| const span = toProtocolTextSpanWithContext(textSpan, contextSpan, scriptInfo); | |||
| const lineSpan = scriptInfo.lineToTextSpan(span.start.line - 1); | |||
| const lineText = scriptInfo.getSnapshot().getText(lineSpan.start, textSpanEnd(lineSpan)).replace(/\r|\n/g, ""); | |||
| if (lineSpan.length > 1024) { | |||
There was a problem hiding this comment.
Since 1024 is just a number I made up, it may need to be tuned. That would be easier if it were a constant.
There was a problem hiding this comment.
To avoid using a magic number, it would be better to create a new variable with a proper name and store 1024 into it.
| const lineText = scriptInfo.getSnapshot().getText(lineSpan.start, textSpanEnd(lineSpan)).replace(/\r|\n/g, ""); | ||
| if (lineSpan.length > 1024) { | ||
| var start = (span.start.offset > 500) ? (span.start.offset - 501 + lineSpan.start) : lineSpan.start; | ||
| var end = (lineSpan.length -2 - span.end.offset > 500) ? (span.end.offset + 499 + lineSpan.start) : ts.textSpanEnd(lineSpan)-2; // need to ask why length is 2 more |
There was a problem hiding this comment.
I probably said "500", but I meant "half of the max length" (easy, if you have a constant for the max length). Also, you may discover after playing with it, that you want less text before and more text after the reference.
There was a problem hiding this comment.
Usually when we subtract two from the length of a line, it's for \r and \n, but line endings vary by platform, so it's usually necessary to use a helper (we probably have one in core.ts or utilities.ts.
| let tokenAtPosition = getTokenAtPosition(sourceFile, start); | ||
| let tokenAtPositionEnd = getTokenAtPosition(sourceFile, end - 1); | ||
| if (tokenAtPosition.getStart() < start) { | ||
| const nextToken = findNextToken(tokenAtPosition, tokenAtPosition.parent, sourceFile)?.getStart()!; |
There was a problem hiding this comment.
I might call this "nextTokenStart", to make it clearer that it's not a token.
There was a problem hiding this comment.
Does getStart return the position before or after the leading trivia?
There was a problem hiding this comment.
In here, getStart returns the start position of the token that was a result of findNextToken
There was a problem hiding this comment.
But if the token looks like /*comment*/identifier, is getStart before or after the comment?
There was a problem hiding this comment.
Also, the ?. followed by the ! is very questionable. Please handle the undefined explicitly.
Also, if you are calling getStart, feed in the optional sourceFile parameter.
| let tokenAtPositionEnd = getTokenAtPosition(sourceFile, end - 1); | ||
| if (tokenAtPosition.getStart() < start) { | ||
| const nextToken = findNextToken(tokenAtPosition, tokenAtPosition.parent, sourceFile)?.getStart()!; | ||
| start = (nextToken - start) > 10 ? start : nextToken; |
There was a problem hiding this comment.
As this gets more polished, it might help to add comments.
| const precedingToken = findPrecedingToken(end, sourceFile, tokenAtPositionEnd)?.getStart()!; | ||
| end = (end - precedingToken) > 10 ? end: precedingToken; | ||
| } | ||
| lineText = scriptInfo.getSnapshot().getText(start, end).replace(/\r|\n/g, "")!; |
There was a problem hiding this comment.
I wonder if we want to add "..." or something when we truncate?
There was a problem hiding this comment.
Aside from the ... issue, we have a trimString (or possibly trimStringEnd) helper which you should probably use instead.
| const sourceFile = scriptInfo.getDefaultProject().getLanguageService().getProgram()!.getSourceFile(fileName)!; | ||
| let tokenAtPosition = getTokenAtPosition(sourceFile, start); | ||
| let tokenAtPositionEnd = getTokenAtPosition(sourceFile, end - 1); | ||
| if (tokenAtPosition.getStart() < start) { |
There was a problem hiding this comment.
feed in the optional sourceFile parameter to getStart.
| start = (nextToken - start) > 10 ? start : nextToken; | ||
| } | ||
| if (tokenAtPositionEnd.getEnd() > end) { | ||
| const precedingToken = findPrecedingToken(end, sourceFile, tokenAtPositionEnd)?.getStart()!; |
There was a problem hiding this comment.
Same comment with ?./! and the sourceFile argument to getStart
|
Closing this; #51081 was merged instead. |
Fixes #50134