Add client side configuration and cache configuration per file #26

haplo · 2021-12-03T11:56:57Z

Use lru_cache decorator on load_config to remember the black configuration for each file that the plugin processes. If the black configuration changes for any reason (e.g. the user editing the pyproject.toml file for the project) then the LSP server would need to be restarted for python-lsp-black to refresh the configuration.

Support plugin configuration (pylsp.plugins.black namespace)
Add unit test for plugin configuration.
Only enable load_config caching if pylsp.plugins.black.cache_config is set in plugin settings.
Add unit test for config caching.
Support Black's preview features if pylsp.plugins.black.preview is true.

Use lru_cache decorator on load_config to remember the black configuration for each file that the plugin processes. If the black configuration changes for any reason (e.g. the user editing the pyproject.toml file for the project) then the LSP server would need to be restarted for python-lsp-black to refresh the configuration.

Also, use those options to get the line length used by Black.

Also, check that line length can be changed from the client

Also add a test for that.

This was added in Black 22.1.0

ccordoba12 · 2022-03-21T17:46:58Z

@haplo, this is ready for review.

ccordoba12 · 2022-03-26T17:44:02Z

Pinging @haplo again. I hope you can review this one soon because we need it for Spyder 5.3.0, to be released next Wednesday.

haplo · 2022-03-27T09:51:15Z

Pinging @haplo again. I hope you can review this one soon because we need it for Spyder 5.3.0, to be released next Wednesday.

Sorry @ccordoba12, I had a busy week. I'm now reviewing all open PRs and will prepare a release once they are all merged.

haplo · 2022-03-27T09:56:32Z

pylsp_black/plugin.py

+    settings = client_config.plugin_settings("black")
+
+    # Use the original, not cached function to load settings if requested
+    if not settings.get("cache_config", True):


If we default to True users could get confused as to why their changes to black configuration files are not being observed (they would need to restart the LSP server). I think either we default to False or we document configuration caching in README. We should document plugin configuration anyway, so maybe we can go with the second option.

I'm updating the README to document plugin configuration.

Sorry, I thought you wanted this option to be True. I'll change it to False then.

My rationale was that caching the black configuration by default could be confusing to users, as black configuration changes (e.g. in pyproject.toml) would need an LSP server restart.

I'm updating the README with this and other details, I think we can merge after that and prepare the release.

haplo · 2022-03-27T09:59:48Z

tests/test_plugin.py

+    """Return a config object."""
+    cfg = Config(workspace.root_uri, {}, 0, {})
+    cfg._plugin_settings = {
+        "plugins": {"black": {"line_length": 88, "cache_config": True}}


If configuration caching is enabled by default we should have a test that checks that it can be disabled.

I added a test for this since it was part of your TODOs. See test_cache_config below.

ccordoba12 · 2022-03-27T17:28:06Z

@haplo, I addressed your review and also added in my last commit (i.e. 5a3ec80) a hook with all config options supported by this plugin.

haplo · 2022-03-28T09:30:10Z

pylsp_black/plugin.py

+        "plugins": {
+            "black": {
+                # Disable this plugin by default because it's third-party.
+                "enabled": False,


I think this is going to break most users' installations when people upgrade. Would it be too bad if we enabled it by default and documented how to disable it?

My rationale is that if the plugin is installed that means it's intended to be used, so having to set the configuration is another step in the way.

Ok, no problem. I'll change this option to True then. However, both Autopep8 and Yapf are enabled too, so I don't know which one takes precedence.

I haven't had trouble with autopep8 and yapf, maybe because I don't install those features (pip install python-lsp-server[rope,pycodestyle,flake8]). Would be worth checking what happens on a regular install, and either plan code fixes or document in README.

Document configuration caching.

haplo · 2022-03-28T13:51:16Z

@ccordoba12 I pushed my update to README, please take a look.

Only open issue is defaulting enabled to false, I think we would get a lot of angry users if we did that. Can we leave it enabled by default while still supporting the enabled configuration option to disable it?

Also fix a small error in our Readme

ccordoba12 · 2022-03-28T16:08:13Z

Can we leave it enabled by default while still supporting the enabled configuration option to disable it?

Sure, no problem. I did it in my last commit.

haplo · 2022-03-28T18:35:31Z

@ccordoba12 I released version 1.2.0. It would be great to have a more direct channel to communicate, is there something for python-lsp-server that we could use?

ccordoba12 · 2022-03-28T21:49:28Z

I released version 1.2.0

Thanks a lot for your help @haplo!

It would be great to have a more direct channel to communicate, is there something for python-lsp-server that we could use?

You mean like a mailing list or Discourse instance?

haplo · 2022-03-28T21:59:31Z

I released version 1.2.0

Thanks a lot for your help @haplo!

You are welcome, thank you for driving this PR home.

It would be great to have a more direct channel to communicate, is there something for python-lsp-server that we could use?

You mean like a mailing list or Discourse instance?

I was thinking of something real-time, like a Matrix channel or maybe Gitter. That could also be a place for users to ask questions and seek support from the community.

ccordoba12 · 2022-04-04T00:31:06Z

I see. Gitter is a good idea, but I don't have time to participate on it. Would you like me to open a Gitter channel for python-lsp-server?

haplo · 2022-04-08T12:26:32Z

My original idea was to have some real-time communication channel with you and maybe other python-lsp-server maintainers, because right now it's only Github issues. If you use Signal (preferred) or Telegram you can find my phone number in my website, just look at my Github profile.

I still think a Gitter channel would be a good idea to give users a place to talk about python-lsp-server and its plugins. I would check it out from time to time, but also don't plan on participating all the time.

haplo added the enhancement New feature or request label Dec 3, 2021

haplo added this to the v1.2.0 milestone Dec 3, 2021

haplo linked an issue Dec 3, 2021 that may be closed by this pull request

Option to cache black configuration per-file #24

Closed

haplo mentioned this pull request Dec 3, 2021

Option to cache black configuration per-file #24

Closed

ccordoba12 mentioned this pull request Jan 21, 2022

3d Party Plugins couldn't be configured python-lsp/python-lsp-server#155

Closed

ccordoba12 force-pushed the cache-config branch from 0fc4a14 to 4f2e051 Compare March 20, 2022 19:22

ccordoba12 force-pushed the cache-config branch from 4f2e051 to bc56959 Compare March 20, 2022 19:25

Add orig files to gitignore

c814fad

ccordoba12 force-pushed the cache-config branch from bb1e1c4 to 6be2701 Compare March 21, 2022 15:43

ccordoba12 added 2 commits March 21, 2022 10:58

Add client side config options

0e3bda6

Also, use those options to get the line length used by Black.

Update tests after adding client config

568a9f5

Also, check that line length can be changed from the client

ccordoba12 force-pushed the cache-config branch from 6be2701 to 568a9f5 Compare March 21, 2022 15:58

ccordoba12 changed the title ~~Cache black configuration per file~~ Add client side configuration and cache configuration per file Mar 21, 2022

Allow clients to configure if loading Black's config is cached or not

96d8d90

Also add a test for that.

ccordoba12 force-pushed the cache-config branch from b41310f to 96d8d90 Compare March 21, 2022 17:20

Allow clients to configure the preview option

204766c

This was added in Black 22.1.0

ccordoba12 force-pushed the cache-config branch from 39309c7 to 204766c Compare March 21, 2022 17:43

ccordoba12 marked this pull request as ready for review March 21, 2022 17:46

ccordoba12 requested a review from dalthviz March 25, 2022 17:02

dalthviz changed the title ~~Add client side configuration and cache configuration per file~~ PR: Add client side configuration and cache configuration per file Mar 25, 2022

haplo commented Mar 27, 2022

View reviewed changes

ccordoba12 added 2 commits March 27, 2022 12:24

Make cache_config option False per review

0c56c67

Add hook with default configuration options

5a3ec80

ccordoba12 force-pushed the cache-config branch from 750455b to 5a3ec80 Compare March 27, 2022 17:26

haplo commented Mar 28, 2022

View reviewed changes

Add Configuration section to README

6539757

Document configuration caching.

Switch to enabling the plugin by default per review

8de8f96

Also fix a small error in our Readme

ccordoba12 changed the title ~~PR: Add client side configuration and cache configuration per file~~ Add client side configuration and cache configuration per file Mar 28, 2022

haplo merged commit e8cfc43 into master Mar 28, 2022

haplo deleted the cache-config branch March 28, 2022 17:26

Add client side configuration and cache configuration per file #26

Add client side configuration and cache configuration per file #26

Uh oh!

Conversation

haplo commented Dec 3, 2021 • edited by ccordoba12 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ccordoba12 commented Mar 21, 2022

Uh oh!

ccordoba12 commented Mar 26, 2022

Uh oh!

haplo commented Mar 27, 2022

Uh oh!

haplo Mar 27, 2022

Choose a reason for hiding this comment

Uh oh!

haplo Mar 27, 2022

Choose a reason for hiding this comment

Uh oh!

ccordoba12 Mar 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haplo Mar 28, 2022

Choose a reason for hiding this comment

Uh oh!

haplo Mar 27, 2022

Choose a reason for hiding this comment

Uh oh!

ccordoba12 Mar 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ccordoba12 commented Mar 27, 2022

Uh oh!

haplo Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haplo Mar 28, 2022

Choose a reason for hiding this comment

Uh oh!

ccordoba12 Mar 28, 2022

Choose a reason for hiding this comment

Uh oh!

haplo Mar 28, 2022

Choose a reason for hiding this comment

Uh oh!

haplo commented Mar 28, 2022

Uh oh!

ccordoba12 commented Mar 28, 2022

Uh oh!

haplo commented Mar 28, 2022

Uh oh!

ccordoba12 commented Mar 28, 2022

Uh oh!

haplo commented Mar 28, 2022

Uh oh!

ccordoba12 commented Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haplo commented Apr 8, 2022

Uh oh!

Uh oh!

haplo commented Dec 3, 2021 •

edited by ccordoba12

Loading

ccordoba12 Mar 27, 2022 •

edited

Loading

ccordoba12 Mar 27, 2022 •

edited

Loading

haplo Mar 28, 2022 •

edited

Loading

ccordoba12 commented Apr 4, 2022 •

edited

Loading