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: Do not error if no note is under the cursor #4136

Merged
merged 2 commits into from
Mar 18, 2024

Conversation

jvanbruegge
Copy link
Collaborator

Otherwise this would also trigger when trying to go to a function definition.

Otherwise this would also trigger when trying to go to a function
definition.
@jvanbruegge
Copy link
Collaborator Author

@jhrcek this should do it

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.

Perhaps a regression test might be helpful to avoid this in the future?

@jvanbruegge
Copy link
Collaborator Author

How can I assert that an error happened? Is there a test I could copy from?

@jhrcek
Copy link
Collaborator

jhrcek commented Mar 12, 2024

Thanks for the quick fix 🙂

How can I assert that an error happened? Is there a test I could copy from?

I did something like that in https://github.com/haskell/haskell-language-server/pull/4111/files#diff-ecd5eff03edc6cac02343f9fe14aeb61fa616f9069e4b3db048d13affb648d17R70-R75

But I think fendor probably means regression test of the form "if there is no note at given position then the plugin doesn't throw any error". Not sure how to best test something like that.

Otherwise I confirm that this PR fixes my issues with the plugin.

@jhrcek
Copy link
Collaborator

jhrcek commented Mar 17, 2024

I looked into implementing a regression test for this and it's not so easy.
There is a deeper issue in the NotesTest, namely that the results that we get from getDefinitions call in all the tests are combination of results from hls-notes-plugin and results from Ghcide's ghcide-hover-and-symbols plugin, which is added to all test sessions by default.

This means that even if the notes plugin throws an error (like " No note at this position" that this PR avoids throwing) we still get a success response in the tests, because based on this logic, if any plugin returns a success response (which in this case ghcide plugin gives success response with empty list of definitions), we just log the error from notes plugin and return the success response.

So I think we should accept this PR without tests because I don't see an easy way to disable that ghcide plugin that masks notes plugin's errors in tests. This will require some discussion and refactoring of test setup logic.

@soulomoon
Copy link
Collaborator

soulomoon commented Mar 17, 2024

It should be quite common, and I am not even sure we should be giving out warnning,
since if a click on names other than note, it triggers

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

@jhrcek jhrcek merged commit b2b41df into master Mar 18, 2024
39 checks passed
@jvanbruegge jvanbruegge deleted the hls-notes-no-error branch March 28, 2024 12:54
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