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

Use the document link request to link to Rails documentation #258

Closed
egiurleo opened this issue Aug 22, 2022 · 11 comments
Closed

Use the document link request to link to Rails documentation #258

egiurleo opened this issue Aug 22, 2022 · 11 comments
Assignees

Comments

@egiurleo
Copy link
Contributor

We implemented the DocumentLink request in #58 and used it to go to method definition from gem RBIs.

As a next step, let's use this request to link to the correct Rails documentation for methods that come from the rails gem.

When a user hovers over a method that's defined in rails, there should be a link that says "go to documentation." We can use information about the method, such as the class that defines it and its ancestor chain, as well as information about the rails gem (version number) to link to the correct documentation page for that method.

@egiurleo egiurleo added the LSP label Aug 22, 2022
@st0012 st0012 self-assigned this Aug 30, 2022
@st0012
Copy link
Member

st0012 commented Aug 31, 2022

I'm not sure if the implementation in my head is the best solution, so I want to discuss it here:

Link format

Class (or module)

Method

Information required for making the link

  • Gem name (e.g. activesupport): file name (e.g. [email protected])
    • Need it to check if it's a Rails component
  • Gem version (e.g. 7.0.3.1): file name (e.g. [email protected])
    • Need it to form the link
  • Class/Module name
    • Need it to form the link
  • (For methods) method name
    • Need it to form the link's anchor
  • (For methods) if it's a class or instance method
    • Need it to form the link's anchor

Implementation

  1. When the request comes in, extract the gem's name and version from it.
  2. Check if it's a Rails gem. Return if it's not.
  3. Create a namespace stack
  4. Parse the document and
    1. When we enter a class/module definition, create a link and push it to the stack
    2. When we visit a method definition, create a link with the class/module name on the stack + the method name

@egiurleo
Copy link
Contributor Author

@st0012 This sounds like a really good plan to me! I have one question: would it be possible to eliminate the use of the stack by using things like the Object#const_source_location or the Method#owner to discover the namespaces of each const and method?

@st0012
Copy link
Member

st0012 commented Aug 31, 2022

I think it's not possible because we're not actually evaluating the code? We can only access names instead of the objects from syntax tree AFAIK.

@vinistock
Copy link
Member

Oh, it's unfortunate that the classes and modules are a part of the URL. However, we don't need to add document links for every single method exported by Rails. Just providing this for the most common ones will probably already provide quite a bit of value.

What do you think about doing something like this

class DocumentLink < BaseRequest
  BASE_RAILS_DOC_URL = "https://api.rubyonrails.org/v#{RAILS_VERSION}" # read the rails version from the stubs
  METHOD_TO_DOC_URL = {
    "belongs_to" => "classes/ActiveRecord/Associations/ClassMethods.html"
  }

  def visit_command(node) # Not sure if it's command, just an example
    suffix = METHOD_TO_DOC_URL[node.value] # or node.message something like that
    return if suffix.nil? # We don't know the doc URL for this one

    target = "#{BASE_RAILS_DOC_URL}/#{suffix}#method-i-#{node.value}"
    # create link from target
  end
end

I suggest prioritizing the following. You can start by implementing only one of them in an initial PR (e.g.: only belongs_to) to get feedback on the logic. After we settled on that, it's just a matter of adding more entries to the hash in follow up PRs.

  • Association methods (belongs_to, has_many, has_one, has_and_belongs_to_many, scope, default_scope, etc)
  • Validation methods (validates)
  • Callbacks (before_create, before_action, etc)
  • Queries (where, find_by, etc)

@paracycle
Copy link
Member

paracycle commented Aug 31, 2022

Rails API guides have a search mechanism which relies on the data stored in the search_index.js file. The data is basically JSON, which we can use to match method names to discover the full doc URL.

For example, the info hash contains the following for run_callbacks:

  // ...
  [
    "run_callbacks",
    "ActiveSupport::Callbacks",
    "classes/ActiveSupport/Callbacks.html#method-i-run_callbacks",
    "(kind)",
    "<p>Runs the callbacks for the given event.\n<p>Calls the before and around callbacks in the order they were set, …\n"
  ],
  // ...

@st0012
Copy link
Member

st0012 commented Sep 1, 2022

@vinistock I think the white listing approach is not ideal for Rails documentation as there are many methods and components that could use this feature. So the list will end up very long and we'll need to manually select things to be added. I'd like to avoid this if possible.

@paracycle The search_index.js is very interesting and I didn't know it exists until now. For anyone who's curious, the file is generated by rdoc so it's not Rails-specific. I thought about using it as a database, but since most of the data are stored in arrays, we may need to transform it first (or bsearch on elements?).

I'll prototype the proposed flow to see if the current rbi files can provide reliable context for generating the urls. If not, I'll use search_index.js to do that.

@Morriar
Copy link
Contributor

Morriar commented Sep 1, 2022

As another aspect to this issue, can we also consider adding comments to the generated RBIs for DSLs in Tapioca?

Thanks to the DSL compilers, a lot of the methods created through Rails meta-programming get reified in the RBI files and appear when the user overs them.

We could make the DSL compilers attach comments to the generated RBI nodes so it explains where the things come from and points to the documentation of the feature.

@Morriar
Copy link
Contributor

Morriar commented Sep 1, 2022

We can use information about the method, such as the class that defines it and its ancestor chain, as well as information about the rails gem (version number) to link to the correct documentation page for that method.

Consider this:

# file1.rb
class CustomJob < ApplicationJob
  # ...
end
# file2.rb
class SomeJob < CustomJob
  # ...
end

Only considering file2.rb won't give you much information about what you could document inside SomeJob, you would need to resolve CustomJob and then find out that it inherits ApplicationJob.

If we want to rely on the model we will have to know about the whole code base and I fear that anything not based on Sorbet will be too slow.

In the meantime we can prototype with the whitelist approach but we may link things that are unrelated by accident.

@st0012
Copy link
Member

st0012 commented Sep 1, 2022

I thought this link will only be activated in rbi files? Because the current flow is like:

  1. Go to definition via Sorbet
  2. Land on a rbi file
  3. Use the source comment to visit the source

So I think the Rails document link will only happen in the rbi file too:

  1. Go to definition via Sorbet
  2. Land on a rbi file
  3. When on a method, say belongs_to, it'll show the user an option to visit the document

If we try to resolve the method source from application code directly, I think it'd be too inaccurate.

@st0012
Copy link
Member

st0012 commented Sep 1, 2022

After discussing with @vinistock we had these conclusion:

  1. The link will be shown in application code, e.g. app/models/post.rb. Not the rbi files I assumed.
  2. We'll use the search_index file mentioned by @paracycle as the data source.
    • The file will be fetched during LSP initialisation via net/http and be stored inside a constant.
    • The fetching should be fail-safe
  3. Because some methods could be common among libraries, like belongs_to is common in API serialisation tools. We may need to use the current file's path to decide if we should show the link. For example, if it's not under app/models/, we don't show links for anything under ActiveRecord's namespaces.
  4. We'll skip ActiveSupport and ActionView at the beginning because it could be hard to determine if the methods are actually from them.

@joosnaak27
Copy link

Ok is cool

andyw8 pushed a commit to andyw8/ruby-lsp that referenced this issue Mar 2, 2024
…ypescript-eslint/parser-5.40.1

Bump @typescript-eslint/parser from 5.40.0 to 5.40.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants