emacs-lsp / lsp-pyright

lsp-mode :heart: pyright
https://emacs-lsp.github.io/lsp-pyright
GNU General Public License v3.0
291 stars 25 forks source link

feat: provide custom variable for diagnosticSeverityOverrides #105

Closed kassick closed 3 weeks ago

kassick commented 1 month ago

As discussed here, any project-specific setting (pyrightconfig.json or [tool.pyright] in pyproject.toml) will take priority -- discarding the values provided here.

This is a way of providing easy-to-set defaults from within the editor for projects that do not have any specific configuration.

seagle0128 commented 1 month ago

@wyuenho your opinion? Should we revert the previous one, or merge this one?

wyuenho commented 1 month ago

If you are going to add the settings back in, just add them all back but in a new PR so you can make sure all the setting values are correct. The couple of PRs I submitted a couple of months ago removed a lot of settings with outdated values. That's said, either approach is the least of my concern. My concern is basedpyright.

I think merging that basedpyright PR was a mistake.

First of all, Mr Microsoft Technical Fellow is right, there are an infinite number of type narrowing one can do and their resolved types viral out to everything else that touches them, which also leads to Pyright diverges from mypy further, in additional to massively slowing down type checking, which is already a NP-complete problem. Microsoft has all the telemetry from VSCode to help them decide what's worth implementing, let them take care of it.

Second of all, I'm willing to bet a month of salary sooner or later basedpyright will spawn a bunch of exclusive settings that don't exist on Pyright or DetachHead is going to rediscover his head and realize it's not a good idea to add every type check under the sun and abandon the project. Both of which are already a problem as discovered here, notice there's no guard in the defcustom to prevent the users to set a basedpyright value to pyright.

All the basedpyright code should be removed from this project and whoever thinks making an NP-complete SAT solver run slower is a good idea can fork this project in a new name and shove every grain of sand into the gears they like.

kassick commented 1 month ago

If you are going to add the settings back in, just add them all back ... either approach is the least of my concern

I for one agree -- having some but not all customs is kind of a mess. But then maintainers would have to also play whack-a-mole with default values eventually changing, so maybe "no settings at all" may be a better approach (that coming from someone who just opener a PR adding a new custom setting , oh the irony :P)

Another option would be updating the readme with something like

lsp-pyright will use the default settings provided by pyright. Pyright strongly advises using pyproject.toml or pyrightconfig.json to configure project-wise settings.

If you need to override pyright defaults, you can use `lsp-register-custom-settings`

(with-eval-after-load 'lsp-mode
  (lsp-register-custom-settings
    '(("pyrhon.analysis.typeCheckingMode" "strict")
      ("python.analysis.diagnosticSeverityOverrides" (lambda () (ht-from-alist '(("someAnnoyingError" . "warning")))))))) 

My concern is basedpyright ... sooner or later basedpyright will spawn a bunch of exclusive settings that don't exist on Pyright or DetachHead is going to rediscover his head and realize it's not a good idea to add every type check under the sun and abandon the project.

I share these concerns. I'm currently testing basedpyright because of the LSP improvements (docstrings, import code action, inlay hints), not because I dislike pyright linting rules -- but I fear that yes, it will eventually diverge too much from both pyright and mypy.

Maybe lsp-pyright could be kept generic enough to support both out-of-the-box (anyone wanting to test basedpyrigh can customize the server command and lsp-register-custom-settings anything needed).

wyuenho commented 1 month ago

maintainers would have to also play whack-a-mole with default values eventually changing

I'm pretty sure all LSP client maintainers are very used to whack-a-mole at this point :) Let's not go the eglot route, especially when it's not exactly obvious how to set the settings correctly with booleans and vectors/arrays