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

Start separating RubyDocument and Document concerns #2222

Merged
merged 3 commits into from
Jul 2, 2024

Conversation

vinistock
Copy link
Member

@vinistock vinistock commented Jun 21, 2024

Motivation

First step towards #1206

I've been reflecting on what would be necessary to better support other document types in the Ruby LSP (e.g.: ERB, RBS, Slim, etc).

There are many parts of the codebase that will need a bit of restructuring to ensure we are separating concerns in a way that's manageable.

To begin, I'd like to propose a refactor to documents. Currently, the Document class has a lot of Ruby related logic baked in. For example, it assumes that Prism results are going to be available, which does not make sense for a document parsed with RBS or an ERB parser.

This Document parent class should only house basic document functionality, like handling edits, locating a specific position in the source code (not nodes as those are document specific) and caching.

Implementation

To begin the work of decoupling RubyDocument and Document, I have done these 3 (in the order of commits):

  1. Got rid of the comments method. This is specific to the Prism implementation and cannot be generalized for all types of documents. For example, RBS attaches comments to specific declaration objects. Whenever we need the comments, we can just do prism_result.comments
  2. Got rid of the tree method. This is also Prism specific because the parse method returns a ParseResult object. This is again not the case for RBS, which returns an array of declarations. We can also just invoke value on the parse result where needed
  3. Lastly, I made syntax_error? an abstract method. Each document type will need to define how they track if there's a syntax error

Next steps

To fully support multiple document types in the LSP, we will need to:

  1. Make Document a generic class, where the type argument represents the AST that gets returned when we invoke the abstract method parse
  2. Refactor requests to receive all parameters. This will not only avoid request classes that accept too many arguments, but it will also make it easier to differentiate what needs to happen for each document type in each request
  3. Move locate and locate_node to RubyDocument as those are specific to the Prism AST

@vinistock vinistock added server This pull request should be included in the server gem's release notes other Changes that aren't bugfixes, enhancements or breaking changes labels Jun 21, 2024
@vinistock vinistock self-assigned this Jun 21, 2024
@vinistock vinistock requested a review from a team as a code owner June 21, 2024 20:04
@vinistock vinistock requested review from andyw8 and st0012 June 21, 2024 20:04
@vinistock vinistock merged commit 6bb73f1 into main Jul 2, 2024
35 checks passed
@vinistock vinistock deleted the vs/separate_parent_doc_concerns branch July 2, 2024 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
other Changes that aren't bugfixes, enhancements or breaking changes server This pull request should be included in the server gem's release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants