Fix Lsp Symbol Rename Bug #1046
Conversation
There was a problem hiding this comment.
LGTM, but I might give @leviathanbeak the chance to give this a tick in case there was some reason for make_range_end_inclusive I'm overlooking.
Also it looks like the lock file update isn't necessary to land this right? If that's the case, are we able to remove that commit? I think the general approach for maintaining the lock file is to only update if it is strictly necessary for the PR to work (e.g. new dependencies were introduced or some critical bug was patched) or the PR itself is for a dedicated update of all dependencies. Otherwise it's a bit too easy to run into lots of unrelated conflicts between PRs.
JoshuaBatty
force-pushed
the
josh/lsp_rename_bug
branch
from
0ef4ef9 to
c7a42ce
Compare
|
Ok cool, make sense. I'll omit the lock file in future PR's unless it is necessary for the PR to work. @leviathanbeak not sure why this function was once needed? |
|
@JoshuaBatty I don't remember exactly for renaming, but I know for example when hovering over symbols there was a "off by 1" bug, where you would hover over the last character and you would not get anything so I added +1. So I probably added +1 to the rename range when I shouldn't, if this fix passes all the possible scenarios:
then I guess it's fine :) |
|
Is this good to go? Seems like everybody is happy with it and it has been 7 days since last activity. cc @JoshuaBatty @adlerjohn @mitchmindtree |
Sorry, have been sick so haven't had a chance to double-check what Elvis recommended, going to do that now and finish this PR off. |
JoshuaBatty
force-pushed
the
josh/lsp_rename_bug
branch
from
c7a42ce to
ad6ef21
Compare
|
This should be good to go now, have double-checked it passes the scenarios that Elvis brought up. |
closes #1042