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

feat: provide custom variable for diagnosticSeverityOverrides #105

Merged
merged 2 commits into from
Nov 2, 2024

Conversation

kassick
Copy link
Contributor

@kassick kassick commented Oct 15, 2024

No description provided.

@kassick
Copy link
Contributor Author

kassick commented Oct 15, 2024

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
Copy link
Collaborator

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

@wyuenho
Copy link
Contributor

wyuenho commented Oct 16, 2024

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
Copy link
Contributor Author

kassick commented Oct 16, 2024

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
Copy link
Contributor

wyuenho commented Oct 16, 2024

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

@seagle0128 seagle0128 merged commit dd54b3a into emacs-lsp:master Nov 2, 2024
11 checks passed
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 this pull request may close these issues.

3 participants