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

Configuration key and plugin name mismatch #41

Closed
rchl opened this issue Feb 12, 2023 · 10 comments · Fixed by #39
Closed

Configuration key and plugin name mismatch #41

rchl opened this issue Feb 12, 2023 · 10 comments · Fixed by #39
Milestone

Comments

@rchl
Copy link

rchl commented Feb 12, 2023

This plugin is called pylsp_black and it's also the name that pylsp uses to load it:

INFO - pylsp.config.config - Loaded pylsp plugin pylsp_black from <module 'pylsp_black.plugin' from '/Users/rafal/Library/Caches/Sublime Text/Package Storage/LSP-pylsp/lib/python3.10/site-packages/pylsp_black/plugin.py'>

The settings it provides use a black key though. This is problematic because that means that the pylsp.plugins.black.enabled key doesn't work and pylsp expects pylsp.plugins.pylsp_black.enabled instead.

All configuration keys other than enabled work as expected because pylsp uses different code path for reading those.

@chantera
Copy link
Contributor

This issue is relevant to PR #39 I made.
The problem is that the plugin uses different names for the entrypoint and the configuration key.
In the PR, I suggest the plugin should use black, but it's okay to adopt pylsp_black instead as long as the plugin uses a consistent name.

@rchl
Copy link
Author

rchl commented Feb 27, 2023

I believe pylsp goes more in the direction of removing the pylsp_ prefix since it's redundant when the setting key starts with pylsp already. For example: "pylsp.plugins.black.line_length"

@haplo
Copy link
Collaborator

haplo commented Feb 28, 2023

Thank you @rchl for noticing this issue.

Moving to pylsp.plugins.black makes sense to me, I just worry about existing users. Best path forward would be to support both namespaces for a while and display a deprecation warning.

Any thoughts here @ccordoba12 ?

@haplo
Copy link
Collaborator

haplo commented May 19, 2023

@ccordoba12 I would like to move forward with this change, just release it as 2.0.0 as it would be a breaking change. We can add a note on release and README for users to upgrade. Any objections?

@haplo haplo added this to the v2.0.0 milestone May 19, 2023
@ccordoba12
Copy link
Member

I agree with releasing this change in 2.0, but I have one minor objection: could you wait until we release a new Spyder release (5.4.4) that supports it? Otherwise, our Black integration would break because we don't have an upper constraint on python-lsp-black (but I'll add one for a future 3.0 version).

The idea is to release 5.4.4 on mid-June at most, so the wait shouldn't be that long. What do you say?

@haplo
Copy link
Collaborator

haplo commented May 21, 2023

I agree with releasing this change in 2.0, but I have one minor objection: could you wait until we release a new Spyder release (5.4.4) that supports it? Otherwise, our Black integration would break because we don't have an upper constraint on python-lsp-black (but I'll add one for a future 3.0 version).

The idea is to release 5.4.4 on mid-June at most, so the wait shouldn't be that long. What do you say?

I think this is reasonable as this is not an urgent change, we can aim for a python-lsp-black 2.0.0 release in the second half of June.

Any other breaking change that would be worth doing besides pylsp.plugins.black namespace?

@haplo
Copy link
Collaborator

haplo commented Jul 6, 2023

@ccordoba12 How are we looking for this change? Was Spyder 5.4.4 released as planned? Can we go ahead with a python-lsp-black 2.0.0 release including this breaking change?

@ccordoba12
Copy link
Member

Hey @haplo, we're almost ready to release 5.4.4 but we're stuck at the moment because we have some problems with our Apple ID, which is necessary to sign our Mac app.

Hopefully they'll be sorted out soon.

@haplo
Copy link
Collaborator

haplo commented Jul 6, 2023

No worries @ccordoba12, I was just checking. I will be out on and off during the Summer, but I will start work on the 2.0.0 branch, I think we could aim for an August release.

@ccordoba12
Copy link
Member

Ok, sounds good to me. That'll give me time to try to integrate black-machiatto project here to make Black work on ranges (which I'd really like to have in Spyder.)

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 a pull request may close this issue.

4 participants