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

Use a strategy pattern and inject encoder to process semantic tokens #94

Merged
merged 10 commits into from May 13, 2022

Conversation

wildmaples
Copy link
Contributor

@wildmaples wildmaples commented May 12, 2022

Motivation

Related to: https://github.com/Shopify/ruby-dev-exp-issues/issues/518

As suggested by @paracycle on Slack:

So, I have a different opinion on this altogether: Both of these methods are only needed for the particular encoding that is needed to satisfy the LSP spec and are not (and should not be) a part of the SH processing. Both of these methods should be implemented as a decorator on top of the SH request handler that transforms the SH processed list of tokens to the proper encoding. That decorator class can be in an Encoder namespace, for example.

Because #compute_delta is one of the possible encoding for our semantic tokens, we can use a decorator dedicated to encoding to modify the tokens gathered from Requests::SemanticHighlighting.

This PR is to demonstrate the refactor to use the decorator pattern in the semantic highlighting request.

@paracycle Let me know if this was what you envisioned.

Implementation

The primary concern I have is that the requests are not set up to use the decorator pattern. I had to go against the request (BaseRequest) calling convention to make it work.

This is how a request is usually run:

Requests::SemanticHighlighting.run(document)

This is how SemanticHighlighting needs to be run using the decorator pattern:

Encoder::Relative.new(Requests::SemanticHighlighting.new(document)).run

Vini previously had a concern for performance, since this would mean we're doing a two pass on the tokens array. However, I think this pattern is more maintainable, understandable and would allow us to have better tests. I think we can optimize later on if it becomes an issue. Wdyt?

Automated Tests

This is a proof of concept. If this is 👍, there is still additional work to be done to refactor the tests. The SemanticHighlighting class can be tested separately from Encoder::Decorator, which is nice.

Manual Tests

N/A

@wildmaples wildmaples added the chore Chore task label May 12, 2022
@wildmaples wildmaples self-assigned this May 12, 2022
@wildmaples wildmaples requested review from paracycle, vinistock and a team May 12, 2022 01:27
Copy link
Member

@paracycle paracycle left a comment

Choose a reason for hiding this comment

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

I like this refactor a lot, thanks for working on this. I've put in some comments to push the refactor forward and clean it up a little bit more.

lib/ruby_lsp/handler.rb Outdated Show resolved Hide resolved
lib/ruby_lsp/encoder/relative.rb Outdated Show resolved Hide resolved
lib/ruby_lsp/encoder/relative.rb Outdated Show resolved Hide resolved
lib/ruby_lsp/encoder/relative.rb Outdated Show resolved Hide resolved
lib/ruby_lsp/encoder/relative.rb Outdated Show resolved Hide resolved
lib/ruby_lsp/encoder/relative.rb Outdated Show resolved Hide resolved
lib/ruby_lsp/encoder/relative.rb Outdated Show resolved Hide resolved
lib/ruby_lsp/requests/semantic_highlighting.rb Outdated Show resolved Hide resolved
lib/ruby_lsp/requests/semantic_highlighting.rb Outdated Show resolved Hide resolved
lib/ruby_lsp/requests/semantic_highlighting.rb Outdated Show resolved Hide resolved
lib/ruby_lsp/handler.rb Outdated Show resolved Hide resolved
lib/ruby_lsp/encoder/relative.rb Outdated Show resolved Hide resolved
@wildmaples wildmaples changed the title Use a decorator to encode semantic tokens Use a strategy pattern and inject encoder semantic tokens May 13, 2022
@wildmaples wildmaples changed the title Use a strategy pattern and inject encoder semantic tokens Use a strategy pattern and inject encoder to process semantic tokens May 13, 2022
Copy link
Member

@paracycle paracycle left a comment

Choose a reason for hiding this comment

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

This looks really nice. One, final, round of comments.

lib/ruby_lsp/requests/support/semantic_token_encoder.rb Outdated Show resolved Hide resolved
lib/ruby_lsp/handler.rb Outdated Show resolved Hide resolved
lib/ruby_lsp/requests/semantic_highlighting.rb Outdated Show resolved Hide resolved
@wildmaples
Copy link
Contributor Author

There are still tests to be written for the encoder. I'd prefer to merge this PR first and make those changes in a separate PR, if you are all good with that.

@wildmaples wildmaples requested a review from paracycle May 13, 2022 18:37
Copy link
Member

@paracycle paracycle 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, thanks for working on this 🙏

@wildmaples wildmaples merged commit 8304c6b into main May 13, 2022
@wildmaples wildmaples deleted the refactor-sh branch May 13, 2022 18:41
@shopify-shipit shopify-shipit bot temporarily deployed to production May 27, 2022 22:06 Inactive
andyw8 pushed a commit to andyw8/ruby-lsp that referenced this pull request Mar 2, 2024
…pes/node-17.0.35

Bump @types/node from 17.0.34 to 17.0.35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Chore task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants