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

Filter code actions based on prefix, not equality #2146

Merged
merged 2 commits into from
Sep 10, 2021

Conversation

michaelpj
Copy link
Collaborator

@michaelpj michaelpj commented Aug 30, 2021

It's quite unclear in the spec, but in
microsoft/language-server-protocol#970
it's suggested that the intention is that the kinds given in only
should be used as prefix filters of the generated code action kinds.

That is to say, if the client asks for only = [ CodeActionRefactor ],
we should give them all kinds of refactoring code actions, including
those whose kind is CodeActionRefactorInline (because as "hierarchical
strings" they are represented as "refactor" and "refactor.inline").

This is quite important for the client: e.g. I hit this because I wanted
to ask for all the import quickfixes so I could present them to the user
to pick one. But they use various subkinds of "quickfix.import", so
currently you cannot ask for them all (asking for "quickfix.import"
currentl returns nothing!).

The ipmlemention is a little ugly: this needs some helper funcitons in
lsp, which I'll make a PR for separately, but I didn't want to block
this.

@michaelpj
Copy link
Collaborator Author

(My goal here is to be able to make a nice emacs function that pulls out all the import-fixing actions and gets the user to pick one, which requires both this and emacs-lsp/lsp-mode#3071)

@jneira
Copy link
Member

jneira commented Sep 2, 2021

build error:

 [4 of 6] Compiling Ide.Types        ( src/Ide/Types.hs, /home/runner/work/haskell-language-server/haskell-language-server/dist-newstyle/build/x86_64-linux/ghc-8.10.6/hls-plugin-api-1.2.0.1/build/Ide/Types.o, /home/runner/work/haskell-language-server/haskell-language-server/dist-newstyle/build/x86_64-linux/ghc-8.10.6/hls-plugin-api-1.2.0.1/build/Ide/Types.dyn_o )

src/Ide/Types.hs:75:12: error:
Error:     Illegal deriving strategy: newtype
    Use DerivingStrategies to enable this extension
   |
75 |   deriving newtype (Monoid, Semigroup)
   |            ^^^^^^^
cabal: Failed to build hls-plugin-api-1.2.0.1 (which is required by
test:wrapper-test from hls-1.3.0.0 and test:func-test from hls-1.3.0.0).

Copy link
Collaborator

@pepeiborra pepeiborra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks reasonable, it would be great if someone could try it with VSCode and verify that it works as expected

@michaelpj
Copy link
Collaborator Author

michaelpj commented Sep 2, 2021

build error

Seems unrelated? Hopefully merging master will fix it.

This looks reasonable, it would be great if someone could try it with VSCode and verify that it works as expected

I don't believe that it will affect anything in VSCode, but I'll see if I can test that...

@jneira
Copy link
Member

jneira commented Sep 3, 2021

Seems unrelated? Hopefully merging master will fix it.

Are you sure? the pr is touching the file which does not compile, maybe the error comes from imports

 src/Ide/Types.hs:75:12: error:
Error:     Illegal deriving strategy: newtype
    Use DerivingStrategies to enable this extension
   |
75 |   deriving newtype (Monoid, Semigroup)
   |            ^^^^^^^

It's quite unclear in the spec, but in
microsoft/language-server-protocol#970
it's suggested that the intention is that the kinds given in `only`
should be used as *prefix* filters of the generated code action kinds.

That is to say, if the client asks for `only = [ CodeActionRefactor ]`,
we should give them all kinds of refactoring code actions, including
those whose kind is `CodeActionRefactorInline` (because as "hierarchical
strings" they are represented as `"refactor"` and `"refactor.inline"`).

This is quite important for the client: e.g. I hit this because I wanted
to ask for all the import quickfixes so I could present them to the user
to pick one. But they use various subkinds of `"quickfix.import"`, so
currently you cannot ask for them all (asking for `"quickfix.import"`
currentl returns nothing!).

The ipmlemention is a little ugly: this needs some helper funcitons in
`lsp`, which I'll make a PR for separately, but I didn't want to block
this.
@michaelpj
Copy link
Collaborator Author

Are you sure? the pr is touching the file which does not compile, maybe the error comes from imports

Huh. For some reason the pre-commit hook strips a couple of language pragmas which are actually needed. Weird. Fixed.

This looks reasonable, it would be great if someone could try it with VSCode and verify that it works as expected

No change in VSCode as far as I can see. It already seems to list all the quickfixes when you hit Ctrl+., I think possibly it doesn't use the server-side filtering. I can't tell because I can't figure out how to turn on the actual message logging any more... anyway, it seems to still work fine.

(Rebased on master)

@michaelpj michaelpj force-pushed the mpj/code-action-prefix branch from dc8a203 to 683bbd9 Compare September 9, 2021 12:27
@jneira jneira added the merge me Label to trigger pull request merge label Sep 10, 2021
Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@mergify mergify bot merged commit d9d27e5 into haskell:master Sep 10, 2021
michaelpj added a commit to michaelpj/haskell-language-server that referenced this pull request Jan 20, 2022
mergify bot added a commit that referenced this pull request Jan 20, 2022
Nicer version as promised in
#2146.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants