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

hls-notes-plugin: Initial implementation #4126

Merged
merged 13 commits into from
Mar 11, 2024

Conversation

jvanbruegge
Copy link
Collaborator

@jvanbruegge jvanbruegge commented Mar 9, 2024

For some reason I could not reopen the other pull request, so I have to open a new one (#3629). Sorry there were a lot of real live issues compounding a few days ago and closing this was just irrational.

In case you are still interested in this, I would still like to have this merged and would maintain it in the future.

Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

Hi, thank you for coming back for this :)

I am wondering, how expensive the note generation is on a huge code base such as GHC.
I am a little bit inclined to disable this plugin by default, to avoid performance regressions, but given that the rule is only triggered on a Goto Definition request, it might be irrelevant.

Also, you should have to add the configuration for the notes plugin to the configuration integration tests: https://github.com/haskell/haskell-language-server/tree/master/test/testdata/schema and https://github.com/haskell/haskell-language-server/blob/master/test/functional/ConfigSchema.hs, I don't understand why the CI is green wthout that 0.o

@jvanbruegge
Copy link
Collaborator Author

I don't think we need to disable this by default because as you said the notes are only searched for when you do goto definition and your cursor is currently on a note reference. Otherwise nothing happens.
If you use this in GHC, the first time you go to a note it takes a few seconds to index, every subsequent note jump is fast

@jvanbruegge
Copy link
Collaborator Author

For the configuration, is that something that I also need to handle in the plugin itself? Or does the hls infrastructure take care of that?

@fendor
Copy link
Collaborator

fendor commented Mar 9, 2024

For the configuration, is that something that I also need to handle in the plugin itself? Or does the hls infrastructure take care of that?

You should have to add entries to the respective json file, so that I don't forget to update the vscode config during the next release. But we check that in CI, so I don't understand what is going wrong right now. I am looking into it, but I think you are fine right now.

@fendor
Copy link
Collaborator

fendor commented Mar 9, 2024

You are good to go, I documented the bug at #4127

@fendor fendor added the status: needs review This PR is ready for review label Mar 10, 2024
Copy link
Collaborator

@jhrcek jhrcek left a comment

Choose a reason for hiding this comment

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

LGTM

@fendor fendor enabled auto-merge (squash) March 11, 2024 11:23
@fendor fendor merged commit 03d418c into haskell:master Mar 11, 2024
39 checks passed
@jvanbruegge jvanbruegge deleted the hls-notes-plugin branch March 11, 2024 16:58
@jvanbruegge
Copy link
Collaborator Author

thanks :)

@fendor
Copy link
Collaborator

fendor commented Mar 11, 2024

Congrats, sorry it took so long :)

@jhrcek
Copy link
Collaborator

jhrcek commented Mar 12, 2024

Hello @jvanbruegge
today I rebuilt my local hls with this feature merged and I'm running into this issue:

The new notes plugin seems too eager to interpret everything as a note.
I just want to ctrl+v some text 😅
The plugin also shows this error when using "Go to definition" on a function (which still works, but simultaneously shows the "No note at this position" error on the side).
Peek 2024-03-12 07-13

Would you have time to look into this and fix this behavior?
If not, I can take a look.
What I would expect is some kind of silent failure if there is no note at given position, as opposed to error being shown to client.
Now that fendor gave you write access to the repo, you could also join the haskell-language-server matrix channel if you prefer more interactive communication.

@jvanbruegge
Copy link
Collaborator Author

I agree this should be changed, I'm on it. Have you mapped go-to-definition to ctrl? Also how do I join the matrix room?

@jhrcek
Copy link
Collaborator

jhrcek commented Mar 12, 2024

I didn't do any keybinding changes in vscode, Seems like ctrl+click is the default way in vscode to trigger "go to definition"
https://code.visualstudio.com/docs/editor/editingevolved#_go-to-definition

How to join the matrix room:
You can use this link https://matrix.to/#/#haskell-language-server:matrix.org
I randomly picked browser-based app element as my matrix client and joined using that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs review This PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants