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

Remove built-in rope_rename plugin #515

Merged
merged 1 commit into from
Mar 29, 2024

Conversation

doolio
Copy link
Contributor

@doolio doolio commented Jan 20, 2024

This functionality is better served by the pylsp-rope plugin.

Fixes #255

This functionality is better served by the pylsp-rope plugin.

Resolves: python-lsp#255
@krassowski
Copy link
Contributor

That would be a major version bump, right? Some downstreams depend on the rope rename support being there by default.

@doolio
Copy link
Contributor Author

doolio commented Jan 20, 2024

Yes, I should think so. This PR relates to this comment from @ccordoba12 who I trust will decide when this could be considered for release with the appropriate version bump.

@ccordoba12
Copy link
Member

That would be a major version bump, right? Some downstreams depend on the rope rename support being there by default.

A couple of questions:

  • Do you know which projects depend on it?
  • I think there shouldn't be major issues for downstream projects because the only thing they should be doing is enabling rope_rename with a config option, which would stop working. But renames could be handled by enabling jedi_rename instead.

@doolio doolio changed the title Remove rope_rename support Remove built-in rope_rename support Jan 20, 2024
@Shane-XB-Qian
Copy link

Shane-XB-Qian commented Jan 21, 2024 via email

@doolio
Copy link
Contributor Author

doolio commented Jan 21, 2024

if it was enabled by default

No, it is not enabled by default

def pylsp_settings():
# Default rope_rename to disabled
return {"plugins": {"rope_rename": {"enabled": False}}}

and not even listed as a configuration option in CONFIGURATION.md which is what brought it to my attention.

Edit: Nor for that matter is jedi_rename!

@lieryan
Copy link
Contributor

lieryan commented Jan 30, 2024

rope_rename is not currently enabled by default, so the only configuration that breaks is for people who explicitly enabled rope_rename, which I don't suppose there will be many people using them.

pylsp-rope currently do not support renaming. I can add this to pylsp-rope, but will need to make sure that the version constraints is such that people don't install pylsp-rope with rename support on older python-lsp-server that also has conflicting rename support.

@ccordoba12
Copy link
Member

rope_rename is not currently enabled by default, so the only configuration that breaks is for people who explicitly enabled rope_rename, which I don't suppose there will be many people using them.

Agreed. That's why I think if we make a note in the release notes about the removal of that plugin and that people can switch to jedi_rename now and pylsp-rope in the future should be enough.

pylsp-rope currently do not support renaming. I can add this to pylsp-rope

That would be great, thanks @lieryan!

but will need to make sure that the version constraints is such that people don't install pylsp-rope with rename support on older python-lsp-server that also has conflicting rename support.

Ok, so that means you probably need to have this in a published release of python-lsp-server so that pylsp-rope can depend on it, right?

@ccordoba12 ccordoba12 added this to the v1.11.0 milestone Mar 29, 2024
@ccordoba12 ccordoba12 changed the title Remove built-in rope_rename support Remove built-in rope_rename plugin Mar 29, 2024
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks for your help with this @doolio!

@ccordoba12 ccordoba12 merged commit f69ed84 into python-lsp:develop Mar 29, 2024
10 checks passed
@doolio
Copy link
Contributor Author

doolio commented Mar 29, 2024

No problem. I presume nothing further is required from me with this PR?

@ccordoba12
Copy link
Member

Nop, it isn't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Confusion about rename support
5 participants