Skip to content

Fix the code action cache level #382

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 2 commits into from
Dec 12, 2022
Merged

Fix the code action cache level #382

merged 2 commits into from
Dec 12, 2022

Conversation

vinistock
Copy link
Member

Motivation

While pairing on #381, I noticed that we had our cache_fetch invocation at the wrong place for code actions. We want to cache the diagnostics response, so that we don't have to run RuboCop more than once, but we do not want to cache the code actions response since it may change depending on which range is visible in the editor.

Implementation

Removed the cache from the server part, which depends on the range, and wrapped the diagnostics call in the cache.

Verified

This commit was signed with the committer’s verified signature.
vinistock Vinicius Stock
We were trying to cache it at server, but that depends on which range
we receive from the editor. What we need to use the cache for is for
fetching the diagnostics, which does not change based on the range
@vinistock vinistock added the bug Something isn't working label Dec 9, 2022
@vinistock vinistock requested a review from a team as a code owner December 9, 2022 16:16
@vinistock vinistock self-assigned this Dec 9, 2022
Copy link
Member

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

👍
Do we want to add a test case for this? Seems like an easy mistake to make.

Verified

This commit was signed with the committer’s verified signature.
vinistock Vinicius Stock
@vinistock
Copy link
Member Author

Good call. Added a test for it in the latest commit.


# If diagnostics is being properly cached, then we should only see the increment_counter block invoked once for the
# same URI, no matter how many times we invoke it and with which range
RubyLsp::Requests::Support::RuboCopDiagnosticsRunner.instance.stub(:run, increment_counter) do
Copy link
Member

Choose a reason for hiding this comment

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

TIL we can implement an increment counter like this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Minitest stubs accept either a value or a callable as the return value, so you can pass any lambdas to it. It's really convenient.

@vinistock vinistock merged commit 07c33b9 into main Dec 12, 2022
@vinistock vinistock deleted the vs/fix_code_action_cache branch December 12, 2022 15:08
@shopify-shipit shopify-shipit bot temporarily deployed to production December 12, 2022 19:50 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants