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

Fix tooltip errorhandling #1195

Merged
merged 2 commits into from
Nov 16, 2023
Merged

Conversation

pblasucci
Copy link
Contributor

@pblasucci pblasucci commented Nov 6, 2023

Co-authored-by: Jimmy Byrd TheAngryByrd@users.noreply.github.com

WHAT

🤖 Generated by Copilot at 683625a

This pull request refactors and improves some parts of the FsAutoComplete core and LSP server modules, especially related to error handling, type conversions, and option types. It also adds a new test case for the inline hints feature and cleans up some whitespace in the test project file. The main files affected are Commands.fs, ParseAndCheckResults.fs, AdaptiveFSharpLspServer.fs, Common.fs, InlineHintsTests.fs, and FsAutoComplete.Tests.Lsp.fsproj.

🤖 Generated by Copilot at 683625a

The server for F# LSP
Got some refactoring to see
Result to option
And error adoption
With better formatting and speed

🛠️🚀🧹

WHY

Because proper modelling and handling of various program states is important (and the hack session was fun!)

HOW

🤖 Generated by Copilot at 683625a

  • Simplify the return types of TryGetToolTip and TryGetToolTipEnhanced from Result to option and add logging for the cases when no tooltip is found ( link, link, link, link, link, link, link, link)
  • Replace the Result.ofStringErr function with the Result.lineLookupErr function, which formats the error message using the ErrorMsgUtils.formatLineLookErr function, and add comments for the candidates for a better error model (link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link)
  • Remove the unnecessary conversions of the result type to a CoreResponse type in Commands.fs and AdaptiveServerState.fs (link, link, link)
  • Simplify the expressions by removing the Option.ofResult function in Commands.fs (link, link)
  • Add a new file InlineHintsTests.fs, which defines a test case for the inline hints feature of the server (link, link)
  • Remove some empty lines and line breaks from the project file FsAutoComplete.Tests.Lsp.fsproj (link, link)

Co-authored-by: Jimmy Byrd <TheAngryByrd@users.noreply.github.com>

* Replaces spurious hover errors with log messages
* Similar treatment for certain helper functions
* Basic test for pipeline hints (needs more coverage)
file.Source
|> tryGetLineStr pos
|> Result.mapError ErrorMsgUtils.formatLineLookErr
//TODO ⮝⮝⮝ good candidate for better error model -- review!
Copy link
Member

Choose a reason for hiding this comment

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

👀

Copy link
Member

@TheAngryByrd TheAngryByrd left a comment

Choose a reason for hiding this comment

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

Thanks a ton @pblasucci!

@1eyewonder I'm gonna merge this one. Probably want to rebase your branch.

@TheAngryByrd TheAngryByrd merged commit ee65267 into ionide:main Nov 16, 2023
10 of 12 checks passed
@1eyewonder
Copy link
Contributor

Yep I'll get it on later this evening 😄

@TheAngryByrd TheAngryByrd changed the title Results of Amplifying F# Sesssion Fix tooltip errorhandling Nov 17, 2023
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.

3 participants