-
-
Notifications
You must be signed in to change notification settings - Fork 414
Fix notes plugin: avoid crashes on missing note definitions and improve lookup handling #4773
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
base: master
Are you sure you want to change the base?
Fix notes plugin: avoid crashes on missing note definitions and improve lookup handling #4773
Conversation
…ndle lookups safely
jvanbruegge
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is really hard to tell what the actual changes are here. Please do not do any reformatting in the same commit as you do functional changes. That is even ignoring that the changed formatting does not even pass the pre-commit hook from the project.
Please remove all reformatting from this PR, or at least split it into a separate commit
| matchAllText noteRefRegex ln | ||
| where | ||
| atPos c arr = case arr A.! 0 of | ||
| -- We check if the line we are currently at contains a note |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why delete this comment?
| mContents <- liftIO (runAction "notes.getfileContents" state (getFileContents nfp)) | ||
| case mContents of | ||
| Nothing -> | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the extra newlines?
|
Thanks for the feedback
Please let me know if anything else should be adjusted. |
|
@jvanbruegge a heads-up, it might be hard to review this PR because this is at least largely a chatbot's work. You can see similar pull requests to other projects on author's profile. |
|
To clarify, I reviewed the changes myself before submitting the PR. |
Fixes #4763
This PR updates the Notes plugin to safely handle cases where note definitions or references are missing.
So, Instead of throwing internal errors, the plugin now returns
Nullresults as expected by the LSP specs.Changes include:
Graceful handling of missing notes in jump-to-definition and references.
Replacing unsafe lookups with
fromMaybe.Cleaning up unused imports.
Functionality tested with a simple example; jump-to-definition works correctly and missing notes no longer cause errors.