Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: consider priority_selector when using best_session during rename #2538

Merged
merged 10 commits into from
Oct 27, 2024

Conversation

rchl
Copy link
Member

@rchl rchl commented Oct 25, 2024

Pass point to best_session() so that priority_selector is respected.

I've noticed that having LSP-some-sass and LSP-css running at once sends prepareRename to LSP-some-sass but rename to LSP-css.

Having this fix + prioriority_selector defined in LSP-some-sass will fix the issue.

Related to sublimelsp/LSP-some-sass#4

@jwortmann
Copy link
Member

I think the correct way to fix this would be to pass the session name from prepare_provider_session to _on_prepare_result and from there further as an additional argument to when the lsp_symbol_rename command is called a second time. Then if this name is not None, use self.session_by_name instead of self.best_session (in _do_rename) to select the rename session. This would guarantee that it is really the same session.

Otherwise there might be only one of the servers having support for renameProvider.prepareProvider, but another session has a higher priority_selector. In that case, the first session would still be used for the prepare rename request, and then the other session for the rename request.

Passing the point to self.best_session for the renameProvider.prepareProvider is certainly still useful (probably also for various other commands in LSP). I think the code block could be simplified to

point = get_position(self.view, point=args.get('point'))
if not point:
    return None

though I see that in this PR it was just moved from one place to another.


Related to #2538

Infinite loop from self reference detected.

@rchl
Copy link
Member Author

rchl commented Oct 26, 2024

Passing the point to self.best_session for the renameProvider.prepareProvider is certainly still useful (probably also for various other commands in LSP). I think the code block could be simplified to

point = get_position(self.view, point=args.get('point'))
if not point:
    return None

Good that someone still remembers what kind of utility functions we have :)

Though in this case I think we should actually not return since the prepare rename request should not need the point. (Not that I ever see point being None here...)

@rchl
Copy link
Member Author

rchl commented Oct 26, 2024

With the addition of session_name for lsp_rename, now the whole fix for the point doesn't even matter for the sublimelsp/LSP-some-sass#4 case anymore. But of course it's still a good fix.

Copy link
Member

@jwortmann jwortmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though in this case I think we should actually not return since the prepare rename request should not need the point.

Not sure about that. Isn't the whole point of the prepare rename request to check whether a given location is valid to rename?

The control flow of the rename command is quite complicated, but I think here it probably doesn't matter because it would still return None because "placeholder" is not in args, and then it will eventually return from run if the point is None (which means that there is no cursor in the view).

plugin/rename.py Outdated Show resolved Hide resolved
plugin/rename.py Outdated Show resolved Hide resolved
plugin/rename.py Outdated
Comment on lines 148 to 149
if point is None:
return None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be removed

@rchl rchl merged commit f51bbbe into main Oct 27, 2024
8 checks passed
@rchl rchl deleted the fix/best-session branch October 27, 2024 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants