[Previews] Fix computation of out of scope locations #4428
Conversation
codehag
left a comment
codehag
left a comment
There was a problem hiding this comment.
looks pretty good thanks! added a few comments and a question
| const inScope = linesInScope && linesInScope.includes(location.line); | ||
| let inScope; | ||
| const isLineInScope = linesInScope && linesInScope.includes(location.line); | ||
| if (isLineInScope) { |
There was a problem hiding this comment.
lets move this into its own function which returns a value that can be assigned to inScope
here is how it would looks like:
function isInScope(linesInScope, location) {
const isLineInScope = linesInScope && linesInScope.includes(location.line);
if (isLineInScope) {
return true;
}
const startLineLocation = outOfScopeLocations.find(
loc => loc.start.line === location.line
);
if (startLineLocation) {
return startLineLocation.start.column > location.column;
}
const endLineLocation = outOfScopeLocations.find(
loc => loc.end.line === location.line
);
if (endLineLocation) {
return endLineLocation.end.column < location.column;
}
// if none of these conditions are true, return false
return false;
}
// use
const inScope = isInScope(linesInScope, location)| const startLineLocation = outOfScopeLocations.find( | ||
| loc => loc.start.line === location.line | ||
| ); | ||
| if (startLineLocation) { |
There was a problem hiding this comment.
we seem to be using the out of scope locations to determine if the lien is in scope, but we have a linesInScope array -- why was this not working as intended? perhaps the bug is steming from linesInScope not representing the correct information?
There was a problem hiding this comment.
yes, actually linesInScope is not really mandatory here, since we need the column information.
you could write a function which works only with outOfScopeLocations and calculates if a location is out of scope.
I kept linesInScope since it was easier to calculate only in for cases where the line isn't in that array.
hope that makes sense o_O
|
I was working on the issue #4369 from 5 days ago but forgot to leave the message (sorry for causing the double effort). I will suggest another approach for the issue #4369 . Please see the detailed explanation: #4381 (comment) |
|
I think @Fischer-L's solution w/ containsLocation is sufficient here. Happy to re-open if there is still ongoing work. |
4 participants
Associated Issue: #4369
Summary of Changes
Thanks @liorpr for working on this together!
The reason for the bug was that it was ignoring column data. If the token is at the same line where an out of scope range begins, but earlier, then the token was considered out of scope. We added consideration of the column position.
NOTE: the code is dirty, maybe a bit of refactoring is in place. We did it in #goodnessSquad and wanted to create a PR. It works, but it's complicated and can probably be simplified.
Test Plan
updatePreviewprobably needs testing, but we didn't add it.