Confusion about rename support

[rchl]

I can see some references to those:

• jedi_rename: https://cs.github.com/python-lsp/python-lsp-server?q=jedi_rename
• rope_rename: https://cs.github.com/python-lsp/python-lsp-server?q=rope_rename

But none of them is documented.

When I try to rename in a simplest of projects, without setting any jedi_rename or rope_rename options, it appears to work for me (but not for someone else, not sure why yet).

With "pylsp.plugins.rope_rename.enabled": true it appears to work also but incorrectly (the file path is not specified so ST creates new buffer instead of modifying existing one). The log is:

:: --> LSP-pylsp textDocument/rename(6): {'position': {'character': 1, 'line': 0}, 'textDocument': {'uri': 'file:///Users/rafal/Downloads/tpy/test.py.py'}, 'workDoneToken': 'wd6', 'newName': 'xyx'}
:: <<< LSP-pylsp 6: {'documentChanges': [{'textDocument': {'uri': 'file:///test.py.py', 'version': None}, 'edits': [{'newText': "xyx = '111'\nyyy = xyx + '222'", 'range': {'end': {'character': 0, 'line': 2}, 'start': {'character': 0, 'line': 0}}}]}]}

So in conclusion:

• rope_rename is not documented and also seems broken
• jedi_rename doesn't have an option and seems enabled by default - is that right?

I guess my point is:

• should rope_rename be documented, fixed or removed?
• should there be some mention of rename functionality being supported?

[ccordoba12]

jedi_rename doesn't have an option and seems enabled by default - is that right?

I think so

should rope_rename be documented, fixed or removed?

It should be fixed, but we don't have the time to do it right now. So it should remain undocumented, I'd say.

should there be some mention of rename functionality being supported?

If jedi_rename is working, then I'm +1 on doing that.

[rchl]

If jedi_rename is working, then I'm +1 on doing that.

The only issue with it is that it doesn't work without a project and gives confusing log message in that case: sublimelsp/LSP-pylsp#86

[ccordoba12]

The only issue with it is that it doesn't work without a project

I don't think it's possible to make it work in that case.

and gives confusing log message in that case

Could you try to improve it please?

[rchl]

I believe the relevant code is in jedi around here: https://github.com/davidhalter/jedi/blob/695f0832b4518ec412e520dadf227b02b105fe99/jedi/api/refactoring/__init__.py#L75-L115

and the weird warning like:

LSP-pylsp: 2022-08-30 21:38:33,399 CEST - WARNING - pylsp.config.config - Failed to load hook pylsp_rename: '/Users/rafal/Library/Caches/Sublime Text/Package Storage/LSP-pylsp/lib/python3.10/site-packages/mccabe.py' is not in the subpath of '/Applications/Sublime Text.app/Contents/MacOS' OR one path is relative and the other is absolute.

comes from Path.relative_to call.

I believe that in this case "/Applications/Sublime Text.app/Contents/MacOS" refers to what it consider the project path which is the weird part. I understand that there is no workspace folder and thus a specific project path but it's weird that it then fallbacks to what seems to be a working directory. Not sure if that's something that jedi infers itself or if it's passed from pylsp.

[lieryan]

Hi, I am rope's maintainer. I'll try to see if I can fix rope_rename issue in the coming days.

Currently, the built-in rope support in pylsp (i.e. rope_rename.py and rope_completion.py) and pylsp-rope uses separate instance of rope Project. I've been intending to change the built-in implementation to align it with the implementation in pylsp-rope.

With that said, another alternative could be to just move the rope_rename and rope_completion implementation to pylsp-rope (currently, pylsp-rope doesn't implement rename or completion, only refactoring). Once that's done, user can either be required to just install pylsp-rope if they want rope-based rename and completion support, or pylsp could have optional dependency on pylsp-rope when user do pip install python-lsp-server[rope].

What do you think, @ccordoba12?

[ccordoba12]

What do you think, @ccordoba12?

I think it'd be better to integrate everything rope related in pylsp-rope because it'd be easier to develop for you.

or pylsp could have optional dependency on pylsp-rope when user do pip install python-lsp-server[rope]

I think that's a great suggestion and I'm +1 to add it here.

[doolio]

@lieryan any update on this? I ask as I notice rope_rename is not listed in the CONFIGURTION.md and I think as a minimum it should, no?

[doolio]

@lieryan friendly ping.

[lieryan]

@doolio apologies for the delay in responding to your question. I'm copying my response from python-rope/pylsp-rope#14 where I had answered a similar question:

I have created a python-rope/pylsp-rope#15 for this some time ago but had not had the chance to finish it. It was basically just porting the pylsp core's implementation of rope_rename into here. I'm not currently actively working on it so feel free to either pick up the PR and improve on it, or to restart the effort from fresh instead.

Mainly the PR hasn't really been tested on how it interacts if rope rename feature is also enabled in pylsp core. Ideally I think there shouldn't be two separate implementation of rope between pylsp core and pylsp-rope, so removing rope support out of core and into pylsp-rope might be something we want to explore in the future, but this will be something that will need to be coordinated with core pylsp maintainers.

However, in the mean time, if we implement rename in pylsp-core, it's going to be confusing if the user accidentally enabled two separate implementations of rename rope. I think installing pylsp-rope should probably just force disable pylsp core's rename. There's some thinking that needs to be done to plan such transition, if it's even desirable to merge the implementations.

I don't currently use the rename feature from python-lsp-server (my current setup python-mode when doing renames, which internally uses rope) and since jedi_rename is the default anyway and seems to work fine for most people, this is personally not a high priority for me right now with my current capacity. But I'm happy to help if there's anyone interested to pick up the PR and champion it to move it forward.

I don't fully recall the state of the PR when I left it, but IIRC, the PR probably already work in theory, but I don't recall if I ever tested it in an actual editor yet. And there might be some more work needed to be done for multi-file edits that involves renaming files.

[doolio]

Thanks @lieryan. I don't use rope myself (at least not yet, I'm just a beginner). I was more asking if it was intentional that the built-in rope_rename feature was not listed in the pylsp CONFIGURATION.md file. Perhaps, because it is not working as it should? Probably, more of a question for @ccordoba12 than yourself so apologies for the noise and thank you for taking the time to respond.

[ccordoba12]

Probably, more of a question for @ccordoba12 than yourself so apologies for the noise and thank you for taking the time to respond.

I think it'd be a good idea to remove the rope_rename plugin from here and refer to the pylsp-rope one instead.

[doolio]

@ccordoba12 What about the rope_autoimport and rope_completion plugins?

[ccordoba12]

Those need to stay here. pylsp-rope is only focused on refactoring.

[lieryan]

Hi all, rope rename support is now available on pylsp-rope since 0.1.16. Please read the pylsp-rope README on how to enable pylsp_rope.rename since the config option to enable pylsp-rope's rename is different than the builtin plugin and the functionality is disabled by default.

I think there are two options to go forward from here.

1. pylsp can remove rope_rename, and if someone has rope_rename.enabled set, they can be prompted to install pylsp-rope if they want rope rename support.
2. alternatively, make it so that pip install python-lsp-server[rope] pulls pylsp-rope and setting rope_rename.enabled should automatically cause pylsp_rope.rename to be enabled. This also means that python-lsp-server[rope] and python-lsp-server[all] would also enable all of the other pylsp-rope's codeactions.

Let me know if either of those sounds reasonable.

[ccordoba12]

Hi all, rope rename support is now available on pylsp-rope since 0.1.16

That's really good news! Thanks for letting us know about it @lieryan.

I think there are two options to go forward from here.

I'd prefer to go with option 1. It's been customary for us to not give an option to install extras with third-party pylsp plugins. Could you take care of that?

[ccordoba12]

@lieryan, sorry, I forgot that @doolio already opened a pull request for this. So, I'll merge it and include that change in our next version (1.11.0), to be released later today.