Skip to content

Ability to hide warnings in Language Server#7379

Closed
tm1000 wants to merge 3 commits intovimeo:4.xfrom
tm1000:feature/hide-warnings-in-language-server
Closed

Ability to hide warnings in Language Server#7379
tm1000 wants to merge 3 commits intovimeo:4.xfrom
tm1000:feature/hide-warnings-in-language-server

Conversation

@tm1000
Copy link
Copy Markdown
Contributor

@tm1000 tm1000 commented Jan 12, 2022

As it is currently warnings (eg if you have level set to 4 then levels 1-3 will be warnings in your IDE) are shown in IDEs utilizing language server. Users have requested the ability to disable that (psalm/psalm-vscode-plugin#16)

This PR adds the ability to hideWarnings but it also follows the Language Server Protocol to ask the client what the current configuration is. This allows this setting to be changed without having to restart Psalm Language Server (which you have to do for all other settings because they are CLI flags). https://microsoft.github.io/language-server-protocol/specification#workspace_configuration

@tm1000
Copy link
Copy Markdown
Contributor Author

tm1000 commented Jan 12, 2022

Using version ^2.0 for php-standard-library/psalm-plugin
./composer.json has been updated
Running composer update php-standard-library/psalm-plugin
Loading composer repositories with package information
Updating dependencies
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - azjezz/psl is present at version 1.9.x-dev and cannot be modified by Composer
    - php-standard-library/psalm-plugin 2.0.0 conflicts with azjezz/psl <2.0.
    - Root composer.json requires php-standard-library/psalm-plugin ^2.0 -> satisfiable by php-standard-library/psalm-plugin[2.0.0].

You can also try re-running composer require with an explicit version constraint, e.g. "composer require php-standard-library/psalm-plugin:*" to figure out if any version is installable, or "composer require php-standard-library/psalm-plugin:^2.1" if you know which you need.

Installation failed, reverting ./composer.json to its original content.

@orklah
Copy link
Copy Markdown
Collaborator

orklah commented Jan 15, 2022

The error on CI is fixed.

@weirdan I think this is a BC break. Do you agree?

@tm1000 could this be added on Psalm 5 or is it important for every version?

@tm1000
Copy link
Copy Markdown
Contributor Author

tm1000 commented Jan 15, 2022

Yeah @orklah v5 is fine. Should I scope it to that?

@orklah
Copy link
Copy Markdown
Collaborator

orklah commented Jan 15, 2022

yeah, if version is not an issue, please target master branch

@tm1000
Copy link
Copy Markdown
Contributor Author

tm1000 commented Jan 15, 2022

@orklah is there an issue for what the roadmap of v5 is?

@orklah
Copy link
Copy Markdown
Collaborator

orklah commented Jan 15, 2022

There's a discussion here: #7171 but there is no clear roadmap defined.

@tm1000
Copy link
Copy Markdown
Contributor Author

tm1000 commented Jan 15, 2022

@orklah ok thanks. That actually helps a lot.

I think I can slip in a few more features into v5 or maybe at least upgrade amp to whatever version supported 7.4 if it's not already up to date here

@orklah orklah marked this pull request as draft January 19, 2022 19:38
@orklah
Copy link
Copy Markdown
Collaborator

orklah commented Jan 19, 2022

I'm putting this in draft for now while you change target

@weirdan
Copy link
Copy Markdown
Collaborator

weirdan commented Jan 23, 2022

I think this is a BC break

Doesn't look like one to me. From what I understand, older clients would ignore the configuration request (or return an error). What makes you think it's a break?

@orklah
Copy link
Copy Markdown
Collaborator

orklah commented Jan 23, 2022

@weirdan
Copy link
Copy Markdown
Collaborator

weirdan commented Jan 23, 2022

The class is internal though, so we can change it any way we want.

@tm1000
Copy link
Copy Markdown
Contributor Author

tm1000 commented Jan 23, 2022

There aren't any breaking changes because the parameters are dynamic. It's how the language server is written. Any params sent over STDIN (from the language server) are then send to methods/classes using reflection. Because reflection is used it can never really break as long as you set sane parameter defaults. The ordering of parameters could also be swapped because the alignment is determined by reflection as to where the values go to each parameter

I realize this is confusing but it's how Felix Becker wrote the language server which is centered around AMPPHP. Once you get your head around it it starts making sense in a way.

but if I apply this to v5 then I can just move more configuration settings into this for IDEs so that we don't have to restart psalm every time. So I'm fine with shooting for more work on the language server in v5

@orklah
Copy link
Copy Markdown
Collaborator

orklah commented Jan 24, 2022

if there's no BC break, I'm fine with both :)

@tm1000
Copy link
Copy Markdown
Contributor Author

tm1000 commented Jan 24, 2022

Should new features be in v5 moving forward? I'm quite fascinated by what Matt was able to achieve using php and creating a language server out of Psalm and I think there's a lot of way to go as long as you both keep approving my PRs ;) (additionally Matt/Weirden gave me admin on the psalm vscode plug-in as well)

@weirdan
Copy link
Copy Markdown
Collaborator

weirdan commented Jan 24, 2022

Should new features be in v5 moving forward?

For now, features can target either. Once v5 is released we will close v4 for new features.

@tm1000
Copy link
Copy Markdown
Contributor Author

tm1000 commented Apr 26, 2022

See: https://github.com/tm1000/psalm/tree/feature/upgrade-lsp

Which will make it's way in here once I get unit tests added

@tm1000
Copy link
Copy Markdown
Contributor Author

tm1000 commented Dec 20, 2022

Superseded by #8960

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants