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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix handle resolve multiple methods #1380

Merged
merged 5 commits into from
Feb 14, 2024

Conversation

snutij
Copy link
Contributor

@snutij snutij commented Feb 13, 2024

Motivation

Closes: #1343

Implementation

When we invoke resolve_method, do not pick only the first match, bot return an array containing all matches.

  • replace find by select
  • return an empty array when nothing match (feel free to challenge this point, as the interface change)

Handle in all requests using this resolve following advices:

  • merge documentation for hover
  • push all Location for definition
  • merge documentation for signature help

Question: On signature_help request, we could propose multiple signatures, with for each signature information the related documentation and parameters. I did a try, it changes the interface, to allow user changing signature with arrow. You can find an example from VS Code documentation here. Do you think it can be better or bring some unwanted complexity? Idk how it's handled by other IDE.

Automated Tests

Added tests in all request and indexer, again, if missed some useful cases, just say it 馃憤

Manual Tests

Given this code:

# file 1
module Func
  def start
    something()
  end

  # Does something interesting
  def something(argument_name)
    puts argument_name
  end
end

# file 2
module Func
  # Does something interesting
  def something(argument_with_other_name)
    puts argument_with_other_name
  end
end

For hover on something:
image

For go to definition on something:
image

For signature_help on something:
image

@snutij snutij requested a review from a team as a code owner February 13, 2024 23:15
Copy link
Member

@vinistock vinistock 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 great!

lib/ruby_indexer/lib/ruby_indexer/index.rb Outdated Show resolved Hide resolved
Copy link
Member

@vinistock vinistock left a comment

Choose a reason for hiding this comment

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

Awesome! 馃殌

@vinistock vinistock merged commit 64b63bb into Shopify:main Feb 14, 2024
16 checks passed
@vinistock vinistock added the bugfix This PR will fix an existing bug label Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This PR will fix an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle same method name in multiple files
2 participants