search

python-lsp / pylsp-mypy

Mypy plugin for the Python LSP Server.
MIT License
118 stars 35 forks source link

Implementation of `config_sub_paths` does not work with editor integration #51

Closed hetmankp closed 1 year ago

hetmankp commented 1 year ago

I wasn't sure where best to discuss this since merge request #42 had already been merged, so I decided to open a new issue.

To be clear, I'm not sure how specific the issue is to my setup, let me describe it first: Here I'm running NeoVIM with the coc.nvim extension which is in turn managing pylsp. Now there are settings being passed to pylsp by coc.nvim which are defined in the coc-settings.json configuration file. This unfortunately means those settings are not available when the language server runs its init hook, but they are made available once the language server calls the lint hook. I'd already previously searched through other possible hooks but could not find any others that would make sense here.

This is why the f87b0f0 commit was doing most of its work in pylsp_lint() and not init() (though it would make sense to do the same file search in init() as well and take advantage of memoisation). This is also why the test in f87b0f0 had a different implementation to your other tests, it was specifically trying to catch this use case.

I assume if coc.nvim is interacting with the language server in this way, there may be others doing the same thing. It would make sense for pylsp-mypy to use the most up to date value of config_sub_paths in pylsp_lint() just as it does with other setting values. My implementation in f87b0f0 had used memoisation to limit any potential performance impact this might have (i.e. don't do config file search except when the configuration value affecting it changes).

Richardk2n commented 1 year ago

Sorry it took so long. Can you take a look whether 00033691bb817e67e8fe6f79ddb46c64fe62a5f4 looks good to you? It draws strong inspiration from your version but is written a bit differently to prepare should other such settings arise, that need similar treatment. Don't mind the commits afterwards, I had some trouble getting the tests to work properly.

hetmankp commented 1 year ago

This looks great. I've tested it on my system and it behaves correctly now. Thanks for the changes, I'll close the issue.