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

Refactor request handling #777

Merged
merged 7 commits into from
Jul 10, 2024
Merged

Refactor request handling #777

merged 7 commits into from
Jul 10, 2024

Conversation

zachallaun
Copy link
Collaborator

The goal of this PR is to make request handling on the server simpler and easier to understand.

The commits are mostly atomic, so reviewing commit-by-commit may be best. Notes on some of the changes:

  • Delete Server.Provider.Env, which for a long time has been a struct containing only :project, and instead pass projects directly to handlers. This removes a level of indirection and gets rid of any confusion between it and Ast.Env.
  • Delete Server.Provider.Queue.Supervisor in favor of starting a Task.Supervisor explicitly and moving the couple helper functions directly into the module in which they're used.
  • Stop converting request IDs to strings. According to the LSP spec, request IDs can be string | integer, but from everything I found, implementations should not consider 1 == "1". If there's a reason we need to do this conversion (e.g. to handle a badly-behaving client), we can add it back in, but it seemed like an unnecessary complication.
  • Replace Server.Provider.Queue with Server.TaskQueue, which just operates on IDs and MFAs. This means TaskQueue doesn't have to know or care about handlers. The server now finds the handler for the request before calling TaskQueue.add/2, instead of the queue having to do it. This also means that adding to the queue always succeeds.

@zachallaun zachallaun requested review from scohen and scottming June 19, 2024 13:00
@zachallaun zachallaun force-pushed the za-provider-refactor branch from 0c821ba to 6cacdbc Compare June 25, 2024 13:48
Copy link
Collaborator

@Moosieus Moosieus left a comment

Choose a reason for hiding this comment

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

Tests look good + working for me locally. This is definitely a good stride towards ease-of-contribution.

apps/server/lib/lexical/server.ex Outdated Show resolved Hide resolved
apps/server/lib/lexical/server/task_queue.ex Outdated Show resolved Hide resolved
@scottming
Copy link
Collaborator

Delete Server.Provider.Env, which for a long time has been a struct containing only :project, and instead pass projects directly to handlers. This removes a level of indirection and gets rid of any confusion between it and Ast.Env

It is worth noting that we used client_name in the rename PR. So, will we also need to add a client_name field to the Project struct after this pr merged?

@zachallaun
Copy link
Collaborator Author

zachallaun commented Jul 10, 2024

@scottming What if we remove Server.Provider.Env but, instead of passing project to handlers, we just pass the whole %Server.Configuration{}?

The :client_name comes from the Server.Configuration struct, as does :project. It seems to me that Server.Provider.Env is just a subset, but I don't think we need to hide the configuration struct from modules in the server app.

@zachallaun zachallaun force-pushed the za-provider-refactor branch 3 times, most recently from 8b017e0 to 1778c9e Compare July 10, 2024 13:59
@zachallaun
Copy link
Collaborator Author

@scottming @Moosieus @scohen I think this should be ready to go. Addressed PR feedback and, based on Scott's comment, updated the first commit to pass %Server.Configuration{} instead of the %Project{} struct to request handlers. This way, Scott can base his rename PR off of this, using the :client_name in the config, and we can still remove an unneeded struct (the %Provider.Env{}).

@zachallaun zachallaun force-pushed the za-provider-refactor branch from 1778c9e to e02d953 Compare July 10, 2024 14:19
@zachallaun zachallaun merged commit 0841570 into main Jul 10, 2024
12 checks passed
@zachallaun zachallaun deleted the za-provider-refactor branch July 10, 2024 14:56
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