Skip to content

Filter code actions based on prefix, not equality#2146

Merged
mergify[bot] merged 2 commits into
haskell:masterfrom
michaelpj:mpj/code-action-prefix
Sep 10, 2021
Merged

Filter code actions based on prefix, not equality#2146
mergify[bot] merged 2 commits into
haskell:masterfrom
michaelpj:mpj/code-action-prefix

Conversation

@michaelpj

@michaelpj michaelpj commented Aug 30, 2021

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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

jneira commented Sep 2, 2021

Copy link
Copy Markdown
Member

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).

@pepeiborra pepeiborra left a comment

Copy link
Copy Markdown
Collaborator

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

michaelpj commented Sep 2, 2021

Copy link
Copy Markdown
Collaborator Author

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

jneira commented Sep 3, 2021

Copy link
Copy Markdown
Member

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
Copy Markdown
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

@jneira jneira left a comment

Copy link
Copy Markdown
Member

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