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

Add GotoRelevantFile functionality #2985

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add GotoRelevantFile functionality #2985

wants to merge 3 commits into from

Conversation

jenny-codes
Copy link
Contributor

@jenny-codes jenny-codes commented Dec 13, 2024

Motivation

A stab at #2966. We can now click the "Goto Test" and "Goto Source" from the commands menu and from the explorer view.

Instruction here for those that want to use in in neovim

In the LspAttach event callback, add the following

    vim.api.nvim_create_autocmd('LspAttach', {
      callback = function(event)
        local client = vim.lsp.get_client_by_id(event.data.client_id)

        -- Check that the current server has the goto_relevant_file capability
        if client and client.server_capabilities.experimental and client.server_capabilities.experimental['goto_relevant_file'] then
          local goto_relevant_file = function()
            local params = vim.lsp.util.make_position_params()
            -- Make the goto_relevant_file request to the lsp server 
            client.request('experimental/gotoRelevantFile', params, function(err, result)
              if err then
                vim.notify('Error in GotoRelevantFile: ' .. err.message, vim.log.levels.ERROR)
                return
              end

              if not result or vim.tbl_isempty(result) or #result.locations == 0 then
                vim.notify('No relevant file found', vim.log.levels.INFO)
                return
              end

              -- Goto the file location if only one location is returned
              if #result.locations == 1 then
                vim.cmd('edit ' .. result.locations[1])
                return
              end

              -- If more than one locations are returned, open a selection menu 
              -- If not using fzf, change to telescope or vim.ui.select  
              require('fzf-lua').files {
                cwd = vim.fn.getcwd(),
                cmd = "echo '" .. table.concat(result.locations, '\n') .. "'",
              }
            end)
          end
        end
      end,
    })

and then you can bind goto_relevant_file() to a keymap

Implementation

Implementing it as a custom request for now until vscode starts supporting it officially.

I've studied a few different implementations and after some internet browsing decided to use the Jacaard similarity here to find the most likely path to the desired filename.

Some future opportunities–

  • Right now only the locations with the highest similarity score are returned, but we can perhaps adjust it so we return all the results that has passed a certain threshold (with this particular algorithm the coefficient is between 0 and 1, with 1 meaning the two paths are identical)
  • Right now the only relevant file that we try to find is test file <> source file, but in the future we can perhaps extend it so it returns other kinds of relevant files as well.

Automated Tests

Included

Manual Tests

goto-relevant-file-demo.mp4

@jenny-codes jenny-codes self-assigned this Dec 13, 2024
@jenny-codes jenny-codes force-pushed the goto-test branch 5 times, most recently from ce2a926 to fb30abd Compare January 27, 2025 23:45
@jenny-codes jenny-codes added the enhancement New feature or request label Jan 27, 2025
@jenny-codes jenny-codes changed the title Add GotoTest and GotoSource functionality Add GotoRelevantFile functionality Jan 27, 2025
super()

@workspace_path = T.let(Dir.pwd, String)
@path = T.let(path.delete_prefix(@workspace_path), String)
Copy link
Member

Choose a reason for hiding this comment

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

@vinistock I think it may be convenient to have a URI#to_workspace_relative_path that matches VS Code's Copy Relative Path value. WDYT?

@st0012
Copy link
Member

st0012 commented Feb 7, 2025

Sorry for the delay in reviewing. Can you rebase this against the main branch?

@jenny-codes
Copy link
Contributor Author

@st0012 @andyw8 thanks for the feedback!
@st0012 yes of course I will rebase. No need to apologize–I have not marked it as ready for review (still writing some tests)

@jenny-codes jenny-codes marked this pull request as ready for review February 19, 2025 03:48
@jenny-codes jenny-codes requested a review from a team as a code owner February 19, 2025 03:48
@jenny-codes jenny-codes requested review from andyw8 and st0012 February 19, 2025 03:48
Define and implement the experimental/gotoRelevantFile lsp method. This
request takes in a textDcoument param, and retusn an array of locations
that are related to the input file path.

Currently, the related file paths are
- test files, if the input file is a source file
- source files, if the input file is a test file

Basically it goes between source file <> test file.

This is implemented as a experimental method until something more
official comes along.

class GotoRelevantFileTest < Minitest::Test
def setup
Dir.stubs(:pwd).returns("/workspace")
Copy link
Contributor

@andyw8 andyw8 Feb 19, 2025

Choose a reason for hiding this comment

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

I'm a little wary of stubbing pwd since if some test helper, or minitest itself, uses it then it may break.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you expand on that?

@andyw8
Copy link
Contributor

andyw8 commented Feb 19, 2025

I tried tophatting on code-db for the file shopify_slack_bot/client.rb and it didn't seem to be working.

@andyw8
Copy link
Contributor

andyw8 commented Feb 19, 2025

For the CI spelling failures, you can add those terms to project-words.

@jenny-codes
Copy link
Contributor Author

jenny-codes commented Feb 19, 2025

I tried tophatting on code-db for the file shopify_slack_bot/client.rb and it didn't seem to be working.

@andyw8 can you share your tophatting steps? I tried it on both vscode and nvim and both work as expected. For what's worth I've also been using it on shopify/shopify for a while now and have not encountered issues.

@jenny-codes jenny-codes added vscode This pull request should be included in the VS Code extension's release notes server This pull request should be included in the server gem's release notes labels Feb 19, 2025
@andyw8
Copy link
Contributor

andyw8 commented Feb 19, 2025

@andyw8 can you share your tophatting steps? I tried it on both vscode and nvim and both work as expected. For what's worth I've also been using it on shopify/shopify for a while now and has not encountered issues.

Just tried again and it's working, I must have missed something before.


test_prefix_pattern = /^(#{TEST_KEYWORDS.join("_|")}_)/
test_suffix_pattern = /(_#{TEST_KEYWORDS.join("|_")})$/
test_pattern = /#{test_prefix_pattern}|#{test_suffix_pattern}/
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can store these patterns in constants too? Do we need to recompute them for each request?

input_basename.gsub(test_pattern, "")
else
test_prefix_glob = "#{TEST_KEYWORDS.join("_,")}_"
test_suffix_glob = "_#{TEST_KEYWORDS.join(",_")}"
Copy link
Member

Choose a reason for hiding this comment

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

Same as the above.

end

sig { params(path: String, candidates: T::Array[String]).returns(T::Array[String]) }
def find_most_similar_with_jacaard(path, candidates)
Copy link
Member

Choose a reason for hiding this comment

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

I'd appreciate a comment explaining what jacaard means 😂


sig { returns(T::Array[String]) }
def find_relevant_paths
workspace_path = Dir.pwd
Copy link
Member

Choose a reason for hiding this comment

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

Let's take in GlobalState and use its workspace_path method to get this value. I think Requests::Rename is a good reference.

def initialize(path)
super()

@workspace_path = T.let(Dir.pwd, String)
Copy link
Member

Choose a reason for hiding this comment

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

Let's use GlobalState#workspace_path instead.

sig { returns(T::Array[String]) }
def find_relevant_paths
workspace_path = Dir.pwd
relative_path = @path.delete_prefix(workspace_path)
Copy link
Member

Choose a reason for hiding this comment

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

Isn't @path already the value without workspace path prefix?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request server This pull request should be included in the server gem's release notes vscode This pull request should be included in the VS Code extension's release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants