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

Disable ruby-lsp-rspec's definition listener outside test files #46

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

MaicolBen
Copy link
Contributor

Here is what happens when I add the addon
Screenshot 2024-10-16 at 11 15 51

@@ -42,6 +42,7 @@ def on_call_node_enter(node)
entries.each do |entry|
# Technically, let can be defined in a different file, but we're not going to handle that case yet
next unless entry.file_path == @uri.to_standardized_path
next unless entry.file_path.end_with?('spec.rb')
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 breaks tests that use tempfiles, do you know how can I make tempfiles use spec.rb files? Or do you know any other way to check it's an rspec. I have seen used node.receiver && node.receiver&.slice != "RSpec" but it doesn't work always.

@st0012
Copy link
Owner

st0012 commented Oct 19, 2024

I roughly understand the issue but it can not be caused by the listener you're trying to patch because it only returns definitions that are also from the same file (condition).
This means the duplicated definition you see is returned by ruby-lsp's own definition listener. And since it only happens when you install ruby-lsp-rspec, I guess it's because you have a let(:sync_with_intercom) or subject(:sync_with_intercom) in your spec files?

@MaicolBen
Copy link
Contributor Author

Thank you for replying so quickly!

I roughly understand the issue but it can not be caused by the listener you're trying to patch because it only returns definitions that are also from the same file (condition).

Yes, so both core and LSP rspec return definitions

I guess it's because you have a let(:sync_with_intercom) or subject(:sync_with_intercom) in your spec files?

That doesn't exist at all, this happens with every definition of every file (not if the definition is in another file). Also, how can you explain that if I remove my new check, the test returns 2 definitions instead of 1?

I'm curious about your setup, since I cannot get working this addon without including in the gemfile.

@st0012
Copy link
Owner

st0012 commented Oct 22, 2024

It took me a while to find a reproduceable case in my projects but I can confirm that it is the listener's fault 👍
But I think we can simply not initializing this listener in non-spec files. So the solution would be more like this:

diff --git a/lib/ruby_lsp/ruby_lsp_rspec/addon.rb b/lib/ruby_lsp/ruby_lsp_rspec/addon.rb
index 98d13ff..3cde027 100644
--- a/lib/ruby_lsp/ruby_lsp_rspec/addon.rb
+++ b/lib/ruby_lsp/ruby_lsp_rspec/addon.rb
@@ -64,6 +64,8 @@ module RubyLsp
         ).void
       end
       def create_definition_listener(response_builder, uri, node_context, dispatcher)
+        return unless uri.to_standardized_path&.end_with?("_test.rb") || uri.to_standardized_path&.end_with?("_spec.rb")
+
         Definition.new(response_builder, uri, node_context, T.must(@index), dispatcher)
       end

@MaicolBen
Copy link
Contributor Author

Nice! Thank you, it's because I don't know much how the LSP addon system works. This solution has the same issue with specs than mine, any ideas?

@st0012
Copy link
Owner

st0012 commented Oct 22, 2024

On my side the test failed without the change and passed when the above diff is applied. Can you first push the lib code and we can see what's failing on CI?

@MaicolBen MaicolBen force-pushed the bug/double-definition branch from 3f8d870 to ed0b97c Compare October 22, 2024 14:50
@st0012
Copy link
Owner

st0012 commented Oct 22, 2024

Ah ok I get what you mean now. It's other tests that are failing.

You can do this to create tempfiles that end with specific postfix: Tempfile.new(["", "_fake_spec.rb"])

@MaicolBen MaicolBen force-pushed the bug/double-definition branch from ed0b97c to e13e5c8 Compare October 22, 2024 14:58
@MaicolBen
Copy link
Contributor Author

Nice! Thank you!

@st0012 st0012 added the bug Something isn't working label Oct 22, 2024
Copy link
Owner

@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.

Thank you so much for reporting the issue and the PR 😄

@st0012 st0012 changed the title Prevent double definition with core functionality Disable ruby-lsp-rspec's definition listener outside test files Oct 22, 2024
@st0012 st0012 merged commit 3931cac into st0012:main Oct 22, 2024
3 checks passed
@st0012
Copy link
Owner

st0012 commented Oct 22, 2024

I've cut a release for this: https://github.com/st0012/ruby-lsp-rspec/releases/tag/v0.1.17

@MaicolBen
Copy link
Contributor Author

That was really fast, thank to you for the help!

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.

2 participants