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

Add applyrefact:applyAll command as hie.commands.applyall #168

Closed
wants to merge 1 commit into from

Conversation

expipiplus1
Copy link
Contributor

I've never used typescript before today!

@alanz
Copy link
Collaborator

alanz commented Nov 4, 2019

@expipiplus1 can you rebase this against master, I think the tests may now be passing. Then we can see about merging and another release.

@expipiplus1
Copy link
Contributor Author

done, thanks!

@alanz
Copy link
Collaborator

alanz commented Nov 6, 2019

@expipiplus1 Please resolve the conflict, sorry.

@expipiplus1
Copy link
Contributor Author

@alanz, done, thanks

@alanz
Copy link
Collaborator

alanz commented Nov 8, 2019

@expipiplus1 Sorry to do this to you, but it struck me that it might be more universal to add this as a command exposed by hie, which is then available to all clients.

@expipiplus1
Copy link
Contributor Author

@alanz, sorry I think I'm missing something; isn't that already the case, and this change is just making the applyrefact:applyall command available to vscode?

@alanz
Copy link
Collaborator

alanz commented Nov 11, 2019

@expipiplus1 I think we can do this in a way that does not need specific code in the client to be able to use the feature.

This is a desirable way of doing things, because then it will work across more different clients.

@expipiplus1
Copy link
Contributor Author

Ah I see, I'm happy either way

@jneira
Copy link
Member

jneira commented Jul 2, 2020

Hi, as i am translating the hlint plugin from hie to hls i would like to revisit this one. I think expose applyAll as a command could be handy, even if we register it in the lang server in some way.
It would let users execute it using the command palette, emacs style, or the file editor context menu. Maybe i am used to other ides like eclipse but i expect to see file wide actions in those locations.

@expipiplus1
Copy link
Contributor Author

I think this should be closed, it's pretty out of date now.

@jneira will applyAll be making a return in the new HLS hlint plugin? I think it's the last thing I'm waiting for before switching!

@jneira
Copy link
Member

jneira commented Aug 23, 2020

@expipiplus1 yeah, it is being ported from hie and it will use apply.refact in the same way.
I would like to add it as command so i would let this open, if you dont mind.

@expipiplus1
Copy link
Contributor Author

@jneira I've rebased this to be on top of master. I'm not able to test with vscode, however a very similar patch works well with coc.nvim.

@bubba You had some thoughts about the registering commands in this extension, so your thoughts are appreciated.

@lukel97
Copy link
Collaborator

lukel97 commented Aug 27, 2020

@expipiplus1 These commands are for the hlint code actions right? As long as the language server (HLS) is handling them then the vscode extension shouldn't need to register for them from what I understand

@expipiplus1
Copy link
Contributor Author

@bubba That's what I would have assumed too, however @chemzqm (of coc.nvim) suggested writing an extension before when I had a similar query: neoclide/coc.nvim#1277 (comment)

@lukel97
Copy link
Collaborator

lukel97 commented Aug 27, 2020

Hmm the specification doesn't really say much about capabilities.executeCommandProvider.commands or ExecuteCommandOptions. Presumably this is just to advertise "permanent" commands that can be executed anytime. But I don't think it's a prerequisite for actually executing them is it? Do the hints work without the changes in this PR?

@expipiplus1
Copy link
Contributor Author

Do the hints work without the changes in this PR?

The command to applyAll doesn't appear without this PR.

The hints aren't applying either way at the moment, but that's a separate issue ;)

@alanz
Copy link
Collaborator

alanz commented Aug 28, 2020

Something to bear in mind.

VsCode manages a single registry of commands available to execute. If you open more than one haskell project in vscode, it runs more than one LSP server. The commands they register go into a single table.

To avoid ambiguity, when the haskell language server registers a command, it prefixes it with the process number. So statically registering a command in the extension won't work.

And I once more note that I think this is the wrong thing to do.

@expipiplus1
Copy link
Contributor Author

expipiplus1 commented Aug 28, 2020

Alanz, agree entirely that if this is possible without logic on the extension then that's clearly the way to go. I just have no idea how to do that.

This does however seem to work for me (in coc.nvim and vscode), at least for my simple 1 client 1 server setup.

Perhaps it would be possible to merge (or merge if someone could help with the process number thing) and at a later date remove it when the server advertises the command properly.

@jneira
Copy link
Member

jneira commented Nov 9, 2020

The hlint plugin is already offering an apply all quick fix, so maybe this is not needed @expipiplus1?

@expipiplus1
Copy link
Contributor Author

Yeah, happy to close this. Thanks for the thoughtful feedback all.

@expipiplus1 expipiplus1 closed this Nov 9, 2020
@expipiplus1 expipiplus1 deleted the joe-applyall branch November 10, 2020 08:05
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.

4 participants