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

Swap ListItems CodeAction #537

Closed
wants to merge 14 commits into from

Conversation

Strepto
Copy link

@Strepto Strepto commented Feb 28, 2021

What have i made?

Related to elm-tooling/elm-language-client-vscode#170

I have made a CodeAction to re-order lists. This action can move a list item up or down in the list, and can be invoked by keyboard shortcuts from your tool of choice.

moveactions

What is it missing?

@Strepto Strepto changed the title Swaplistitems codeaction Swap ListItems CodeAction Feb 28, 2021
@Strepto
Copy link
Author

Strepto commented Feb 28, 2021

The current test is broken, and I cannot figure out how I should fix it. Would like a pointer to figure out how to handle the undefined at this line
TypeError: Cannot read property 'changes' of undefined

codeActions[0].edit!.changes![URI.file(baseUri + uri).toString()],

@razzeee
Copy link
Member

razzeee commented Feb 28, 2021

Seems like we don't have tooling to test registerRefactorAction yet. The problem is, that you just let the server know which codeActions there are and the actual edit get's created at a later time. But our tests need to do that manually, which we don't do yet. So the call to create the edit instruction never happens right now.

@razzeee
Copy link
Member

razzeee commented Feb 28, 2021

The new logic isn't perfect, as it should be separated some more, but should be fine for now.

@Strepto
Copy link
Author

Strepto commented Mar 2, 2021

Thanks! I'll look into improving this when I have some time to spare.

@Strepto
Copy link
Author

Strepto commented Mar 3, 2021

Thanks for the comments! I'll try working on refactoring next, as I think the logic works better now after I have tested it. Also easier to refactor now that I have some Okish tests!

@razzeee
Copy link
Member

razzeee commented Mar 3, 2021

I would like to test some cases where a CodeAction should NOT be available. Such as "Move list item up" not being available on the first item. And in this particular test no Move list item should appear.)

Does this seem like something I/we could extend the testCodeAction code with @razzeee ?

Should be possible, we already do this for similar tests (diagnostics), not sure if codeActionTests have the capability yet.

Base automatically changed from master to main March 10, 2021 23:06
@Strepto
Copy link
Author

Strepto commented Jun 19, 2021

I have been monitoring microsoft/language-server-protocol#724 for any updates but no news yet.

Without cursor controll I feel this does not yet feel as usable as it should be.

I'll close this for now. But I will reopen or create a new PR once the protocol has this support 🚀.

@Strepto Strepto closed this Jun 19, 2021
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.

2 participants