Skip to content

Plugin tutorial, more changes #4570

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

Merged
merged 8 commits into from
May 23, 2025
Merged

Conversation

dschrempf
Copy link
Contributor

@dschrempf dschrempf commented Apr 23, 2025

Some changes are mine, but many have been cherry picked and amended from PR #3655 by Christian Georgii [email protected]. @cgeorgii

This PR now includes most changes of #3655. I did not include the last sections which are behind a "what shall we do with those" comment in the aforementioned PR. I am not sure how to deal with those.

Another issue is that the link to the gist is outdated (and still links to the original one by Pepe Iborra). So I think we either have to remove the link or write a new gist and replace the link. What are your opinions?

I did not implement the literate file stuff, it seems too complicated to achieve in one PR.

Supersedes #3655.

@dschrempf dschrempf force-pushed the plugin-tutorial-2 branch 3 times, most recently from 39a170a to 519de6e Compare April 27, 2025 19:22
@dschrempf dschrempf marked this pull request as ready for review April 27, 2025 19:32
@dschrempf dschrempf requested a review from michaelpj as a code owner April 27, 2025 19:32
Copy link
Collaborator

@VeryMilkyJoe VeryMilkyJoe left a comment

Choose a reason for hiding this comment

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

Thanks for picking up this PR!
I have some comments and suggestions.

Comment on lines 105 to 108
* Request handlers respond to requests from an LSP client. They must fulfill these requests as quickly as possible.
* Notification handlers receive notifications from code not directly triggered by a user or client.
* Rules add new targets to the Shake build graph. Most plugins do not need to define new rules.
* Commands are an LSP abstraction for user-initiated actions that the server handles. These actions can be long-running and involve multiple modules.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the rules could be discussed later on instead of here.
I would rather write this as:

Suggested change
* Request handlers respond to requests from an LSP client. They must fulfill these requests as quickly as possible.
* Notification handlers receive notifications from code not directly triggered by a user or client.
* Rules add new targets to the Shake build graph. Most plugins do not need to define new rules.
* Commands are an LSP abstraction for user-initiated actions that the server handles. These actions can be long-running and involve multiple modules.
There are two main groups of communication via the Language Server Protocol, namely:
* A request-response type interaction where one party sends a message which the other party has to respond to somehow
* `pluginHandlers` handle client requests and provide a response
* Example: When you want to format a file the client sends the [`textDocument/formatting`](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_formatting) to the server which triggers the server to format the file, the server then responds with the formatted file content
* A notification, which is a one-way interaction, where one party sends a message without expecting any response
* `pluginNotificationHandlers` handle notifications sent from the client to the server
* `pluginCommands` are a special type of notification, these are also sent to the server but can be triggered by the user instead of happening automatically
* Example: Whenever you modify a Haskell file, the client sends a notification, informing HLS about how the file has changed
> **Note:** The LSP client and server can both send requests or notifications to the other party.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good summary of the LSP communication scheme. I moved part of the summary before "The anatomy of a plugin" into a subsection called "Notes about the Language Server Protocol" and tried to consolidate the rest. I think the text has improved a lot.

Copy link
Collaborator

@VeryMilkyJoe VeryMilkyJoe May 19, 2025

Choose a reason for hiding this comment

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

Thanks, I agree!

```

`runImportCommand` [sends a request](https://hackage.haskell.org/package/lsp/docs/Language-LSP-Server.html#v:sendRequest) to the client using the method `SWorkspaceApplyEdit` and the parameters `ApplyWorkspaceEditParams Nothing edit`, providing a response handler that does nothing. It then returns `Right Null`, which is an empty `Aeson.Value` wrapped in `Right`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe explain why we have an Either type here, and that Left is used for failures?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have moved the description of the Either up to when we introduce CommandFunction. Please have a look.

@dschrempf
Copy link
Contributor Author

Thanks for your detailed review @VeryMilkyJoe , I have this on my radar, just didn't get to it yet!

@dschrempf dschrempf force-pushed the plugin-tutorial-2 branch from 808ddbf to 0730a8c Compare May 18, 2025 10:50
@dschrempf
Copy link
Contributor Author

dschrempf commented May 18, 2025

Hi @VeryMilkyJoe , I have implemented your proposed changes, thank you. Please have a look if the result is to your liking. One more involved task is still left: The LspFuncs problem (CodeLensProvider is also gone). I suggest merging these changes and creating an issue, but it is a suboptimal solution.

EDIT: Ah, and I forgot, I also update the Nix Flake lock file. This changes the default GHC from 9.6 to 9.8 for Nix users. I guess we should outsource this into a separate PR, but just so you know.

Copy link
Collaborator

@VeryMilkyJoe VeryMilkyJoe left a comment

Choose a reason for hiding this comment

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

Looks great thank you, I just spotted a spelling mistake!

@dschrempf dschrempf force-pushed the plugin-tutorial-2 branch from 222c35a to 776301a Compare May 19, 2025 12:24
@fendor fendor requested review from santiweight and wz1000 as code owners May 19, 2025 16:03
@fendor
Copy link
Collaborator

fendor commented May 19, 2025

I pushed an update where we are compiling the plugin-tutorial.md using HLS head. The implementation uses markdown-unlit and requires a symlink to plugin-tutorial.lhs.

In the process, I had to re-implement large parts of the code in the tutorial, which I think is a net-positive.

@dschrempf
Copy link
Contributor Author

That's great, thank you! I focussed on the text, and mostly copied the code over, so it's great if we can fix that too!

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.

LGTM, thanks @dschrempf for pushing this over the finish line!

@fendor fendor force-pushed the plugin-tutorial-2 branch from f4153bd to 6c22e2c Compare May 23, 2025 13:54
@fendor fendor enabled auto-merge (squash) May 23, 2025 13:56
@fendor fendor disabled auto-merge May 23, 2025 16:25
@fendor fendor enabled auto-merge (squash) May 23, 2025 16:25
@fendor fendor disabled auto-merge May 23, 2025 16:26
@fendor fendor merged commit 1b11a8f into haskell:master May 23, 2025
38 of 41 checks passed
@dschrempf
Copy link
Contributor Author

Wuhuu! 🎉

@dschrempf dschrempf deleted the plugin-tutorial-2 branch May 25, 2025 10:50
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