- Notifications
You must be signed in to change notification settings - Fork91
Introduce theStatusBarItem#1237
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 ourterms of service andprivacy 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?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
I don't know how to review this huge patch, the best way is probably to check it out and trying to understand how the extension is now structured. The commits themselves are somewhat self containing, at least I tried to make sure they are. Happy for any feedback, otherwise I will just merge it in a couple of weeks and deal with any fallout. |
I just skimmed over it. I have no idea what's going on. What did you do to the code? 😭 Can we at least make a prerelease/alpha for this before hitting users with a brick? |
It is more or less a rewrite :) It is less a rewrite, as I am not re-implementing new functionality, except for the config parsing, but I am fixing a couple of lints, removing unused functions, introducing some abstractions, more functions and move them to separate files. It is probably impossible to review from the GitHub page, you'd have to check it out, but Ibelieve it is easier to read now. |
Includes the arguments passed to the server, the logging configuration,working directory, anything that is supposed to stay constant during theexecution of a single HLS instance. Note, it is not promised to stayconstant over restarts.
This 'StatusBarItem' improves the discoverability of the most importantinteractions between the user and the extension.This includes debug information, such as the version of the extension,showing the output panel to find the logs more easily, but also torestart all running Haskell Language Server binaries and the extensionitself.
| `---\n\n` + | ||
| `[$(terminal) Open Logs](command:${constants.OpenLogsCommandName} "Open the logs of the Server and Extension")\n\n` + | ||
| `[$(debug-restart) Restart Server](command:${constants.RestartServerCommandName} "Restart Haskell Language Server")\n\n` + | ||
| `[$(refresh) Restart Extension](command:${constants.RestartServerCommandName} "Restart vscode-haskell Extension")\n\n`, |
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.
| `[$(refresh) Restart Extension](command:${constants.RestartServerCommandName} "Restart vscode-haskell Extension")\n\n`, | |
| `[$(refresh) Restart Extension](command:${constants.RestartExtensionCommandName} "Restart vscode-haskell Extension")\n\n`, |
| logger.error(`${e.stack}`); | ||
| } | ||
| } | ||
| await handleInitializationError(e, logger); |
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.
| awaithandleInitializationError(e,logger); | |
| awaithandleInitializationError(e,logger); | |
| clients.delete(clientsKey); |
otherwise slot will be forever taken
| resolve: (value: string | PromiseLike<string>) => void, | ||
| reject: (reason?: HlsError | Error | string) => void, | ||
| ) => void; | ||
| const haskellConfig = workspace.getConfiguration('haskell'); |
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.
If it's const, it will stay with initial config value. So if user changes hls path, the change wouldn't take effect
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.
Exactly, that's essentially the main difference between restarting hls and restarting the extension. One merely restarts the already running binary, while the other performs some work to figure out the correct HLS binary. So far, this has been a feature, since restarting HLS is much faster than restarting the whole extension, as GHCup might perform non-trivial work.
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.
Yeah, but it currently stay the same even if user restarts extension via status bar. I do have small diffhere that reloads those globals on restartExtension
| @@ -15,6 +14,6 @@ | |||
| "strictNullChecks": true, | |||
| "strictBuiltinIteratorReturn": false | |||
| }, | |||
| "include": ["./src/**/*.ts", "./test/**/*.ts"], | |||
| "include": ["./src/**/*.ts", "./test/**/*.ts", "eslint.config.mjs"], | |||
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.
is fileeslint.config.mjs needed for build process? To me it looks like linter config
The primary motivation for this PR is to make the vscode-haskell extension much friendlier for newcomers.
My idea to improve this is a
StatusBarItemthat looks like this:The
StatusBarItemoffers the following actions:serverExecutablePathis not picked up byRestart HLS)My goal would be for the future to extend the features presented here, for example:
redand display options to help the user understand what went wrong. For example, it could offer to re-display the error message, or some kind of "debug this error" functionality.That's the main motivation.
Initially, I felt it was tricky to do the kinds of changes I wanted to implement, so I went on a complete tangent and started refactoring the extension to avoid side effects, ad-hoc logic, non-uniform accesses to configuration and similar.
Thus, large parts of this PR are about improving the extension to make it easier to maintain and extend. The main refactorings are:
ghcupprocess management into dedicated classI am still unhappy with the code path for finding HLS binaries using GHCup. This is rather complicated logic, for which I couldn't extract or find a pattern, yet.
Even without it, I think I am overall improving the readability of most of the extension.