Support document link for Rails documentation

[st0012]

Motivation

Closes #258

Implementation

Automated Tests

Manual Tests

TODOs

• Replace Rails version number with a placeholder
• Mock the Rails document request in tests
• Activate the feature based on file path, e.g. Only check activerecord's documents when the file is under a models/ folder.
• Exclude activesupport and actionview
• Add tests for actionpack, activemodel, and activestorage
• Use better way for searching
• Documentation

Demo

[vinistock]

This is looking great. One final detail: do we need to pull in VCR and webmock for testing it? Would we be able to use a Minitest stub and avoid adding the dependencies? I don't foresee the LSP having too many API requests, so I'm not sure adding the dependencies will pay off.

[st0012]

@vinistock Given the additional logic on processing and filtering the dataset, I want the request's tests to be as realistic as possible. And in that case, the options we have are:

1. Store the document in plain text, load it manually, and return the result with Minitest stub.
2. Use VCR or webmock for that.

I'm ok with doing 1 instead, but I also don't think it's a big problem having vcr as "development" dependency.

[vinistock]

I think we will probably not be using VCR or webmock anywhere other than this one method in document links. That's why I feel like something like this would be simpler

fake_docs = {
  "belogs_to" => ...
}

RubyLsp::Requests::DocumentLink.stub(:rails_documents, fake_docs) do
  RubyLsp::Requests::DocumentLink.new(document).run
end

[st0012]

@vinistock As I said, I don't just want to test it agains a fake hash. The thing we got from search_index.js is a JS code. And we do a bunch of processing on it to make it work with the request, which I think should be tested too.

[vinistock]

Agreed that there's a lot of value in that, but you can also achieve it with a stub, no?

Net::Http.stub(:get_response, js_content) do
  ...
end
```

[st0012]

Yes and I've mentioned that I'm ok with that:

Store the document in plain text, load it manually, and return the result with Minitest stub.

I'm only against testing it with a hash directly

[vinistock]

Two questions that came up while testing this

• Is there an option for placing the link on top of the hover documentation? These Rails methods have a lot of documentation and we end up having to scroll through a lot before finding the links
• Is there a way of disabling cmd click for document links only? Maybe some VS Code configuration? Right now, if you cmd click a method with links you jump to the browser and Sorbet jumps to the RBI file at the same time

[egiurleo]

This is really cool so far! I left some questions and a suggestion!

[vinistock]

I tophatted this again and it's looking great, I'm really excited for it.

The one thing I think we need to find a decent solution for is the overlap of this feature with go to definition by Sorbet. Right now, CMD + click will perform both operations at the same time. You get redirected to the browser with the documentation and when you come back to the editor you're suddenly in the RBI file.

Two possible ideas, but we need to validate that the experience is decent

1. If it's possible, turn off CMD + click for document links
2. Instead of attaching the document link to the entire word, use only the first character. This will make it harder to hit the conflict, but may degrade the document links experience

[st0012]

In terms of the cmd+click behaviour, I couldn't find anything that helps it. Since cmd+click is arguably a client-specific behaviour, I think it's expected that LSP doesn't cover it. But I also don't find anything config on VSCode that can disable it based-on a specific extension.

But it looks like vscode-terraform has reported a similar problem: microsoft/vscode#147447. So the good news is that we're not alone. The bad news is that the issue was opened almost 6 months ago and still has no comments 😅

[st0012]

Close in favour of #313