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 placeholders for all class instance methods #3017

Closed
July541 opened this issue Jul 5, 2022 · 9 comments · Fixed by #3394
Closed

Add placeholders for all class instance methods #3017

July541 opened this issue Jul 5, 2022 · 9 comments · Fixed by #3394
Labels
component: hls-class-plugin level: easy The issue is suited for beginners type: enhancement New feature or request

Comments

@July541
Copy link
Collaborator

July541 commented Jul 5, 2022

In #2920, we've got all class instance methods and dropped them immediately. So it's not hard to pick them up and supply add placeholder for xxx even if they are not the minimal requirements.

The main point is how to display these code actions properly because there may be so many items. Reference: #2920 (comment)

I'm willing to offer help if someone plans to pick this:)

@batkot
Copy link
Contributor

batkot commented Jul 12, 2022

I'd like to give it a try and some help would be more than welcome 😉

@July541
Copy link
Collaborator Author

July541 commented Jul 13, 2022

@batkot Thank advance for your interest!

If I do this, I'll first display all methods in code action and then give them a tidy look for users.

Please feel free to let me know if you meet any troubles!

@batkot
Copy link
Contributor

batkot commented Aug 11, 2022

Finally I managed to find some time to work on this (sorry for delay).

If I do this, I'll first display all methods in code action and then give them a tidy look for users.

Not sure if I'm getting you right, but what I was able to quickly hack is single (well make it 2, counting with signature(s) 😉) code action for adding placeholders for all not implemented type class methods.

image

I'm wondering if some kind of action (or lens?) for adding placeholders for not implemented methods even when minimal requirements are meet would be useful? E.g. creating placeholder for (/=) when (==) is implemented 🤔

@July541
Copy link
Collaborator Author

July541 commented Aug 12, 2022

It may be quite a lot of methods for one class, import them at one time looks crazy, I prefer something like this, a secondary list to select methods one by one.
image

I'm wondering if some kind of action (or lens?) for adding placeholders for not implemented methods even when minimal requirements are meet would be useful? E.g. creating placeholder for (/=) when (==) is implemented

In most cases, it's useless, but implementing this to supply for corner cases is worth I think.

@batkot
Copy link
Contributor

batkot commented Sep 16, 2022

Hi again. Time for monthly update, I guess 😅

I prefer something like this, a secondary list to select methods one by one.

What kind of secondary list are we talking about? Some kind of "expandable/searchable submenu" or just appending additional items (one per not implemented method)? I'm not sure how to achieve the former (if it's possible, I'd love some directions) and the latter can result in pretty big code action lists (e.g. list of 34 items for Foldable feels overwhelming 😕) Maybe breaking it into "2 stages" is a good idea? First we could suggest placeholders to fulfil minimal requirements and when they're met we could suggest placeholders for other missing methods 🤔 Or maybe as it was suggested in linked PR, we should drop with signature(s) variant of code action, since it would cut in half number of items on list.

@July541
Copy link
Collaborator Author

July541 commented Sep 17, 2022

Some kind of "expandable/searchable submenu" or just appending additional items (one per not implemented method)?

The first one I think. I don't know what is in vscode(I tried to go through the lsp doc but found nothing), but some other language servers have this.

At least we can have only one code action to add unnecessary methods once, though people won't use it very often.

@batkot
Copy link
Contributor

batkot commented Nov 24, 2022

I did some digging and as you said it looks like LSP doesn't support any kind of CodeAction groupings/filtering. I found this issue for LSP extension, that HLS might support. However it seems a bit forgotten and it's not supported by vscode (and probably other IDEs) by default, so it's probably not worth it.

Other LSPs implement their own ux in vscode extensions (e.g. Java, Rust. Roslyn*). *This one is probably for VS.

One low hanging fruit, that kind of works, would be to use different CodeActionKind for implementing minimal requirements and other methods, e.g. vscode separates QuickFixes, Refactor and Source CodeActionKinds into different context menus. We could use that to our advantage, it could look more or less like this:

Implement minimal requirements in QuickFixes Ctrl + .:
image

Implement any missing method in Source Action Context Menu:
image

However it still gets messy for Foldable. I'd just drop the with signature(s) action here:
image

Downside is that other clients might not group CodeActions by their Kind (e.g. CoC):
image

Sorry for lengthy post. TL;DR:

We have few options:

  1. Use source.* CodeActionKind for "implement missing method" CodeAction to separate it from QuickFixes in VSCode
  2. Implement custom UI/action handling in VSCode extension - similiar to what vscode-java and rust-analyzer do.
  3. Create just single action for adding placeholder for all missing class methods.
  4. Other ideas? 🤔

Both 1. and 2. are VSCode centric and could lead to CodeAction spam in other IDEs, but that could probably be mitigated by configuration/clientCapabilities 🤔

Edit:
I fiddled with vscode-haskell a bit and if we went with 2nd option, we could end up with something like this
image

But, as said, it would require either custom code action handling or custom action that does all the funky stuff (similiar to import) and breaking/extending LSP contract.

@July541
Copy link
Collaborator Author

July541 commented Dec 3, 2022

Option 2 looks better to me.

Any suggestion? @michaelpj

@michaelpj
Copy link
Collaborator

Option 2 seems to me like a lot of work for the benefit of being able to insert placeholders for only a subset of class methods. And the fact that it won't work and will have actively bad UX in non-VSCode clients seems pretty bad to me. So I'd probably go with option 3 for now.

batkot added a commit to batkot/haskell-language-server that referenced this issue Dec 11, 2022
batkot added a commit to batkot/haskell-language-server that referenced this issue Dec 11, 2022
batkot added a commit to batkot/haskell-language-server that referenced this issue Dec 11, 2022
batkot added a commit to batkot/haskell-language-server that referenced this issue Dec 12, 2022
batkot added a commit to batkot/haskell-language-server that referenced this issue Dec 12, 2022
batkot added a commit to batkot/haskell-language-server that referenced this issue Dec 12, 2022
@mergify mergify bot closed this as completed in #3394 Dec 27, 2022
mergify bot pushed a commit that referenced this issue Dec 27, 2022
* #3017 WIP: Add Action for all missing class methods

* #3017 WIP: Filter out equivalent suggestions

* #3017 Fix CodeAction title, adjust tests

* #3017 Rename functions

* #3017 Update demo gifs

* 3017 Stylish-haskell

Co-authored-by: Lei Zhu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: hls-class-plugin level: easy The issue is suited for beginners type: enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants