Skip to content

Allow extensions to contribute to F1 show help topic #785

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

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

Conversation

dfalbel
Copy link
Contributor

@dfalbel dfalbel commented Apr 29, 2025

Adresses posit-dev/positron#7430

The goal of this PR is to allow extensions to add support for F1 show help topic for objects.
It's specifically targetting reticulate objects, but could be used by eg, R6 objects to go to their documentation.

There are 3 sets of changes:

  • Include the extract operator in the topic node, so that if you have object$fu@n(), (@ is marking the cursor position) and press F1, then we're calling showHelpTopic with topic = "object$fun". I don't think this brings any downside because currently this would just send foo, and if foo is actually a function in the global namespace, it would actually show the wrong documentation page. eg object$abs shows the documentation for base abs.

  • This is the core change. showHelpTopic now evaluates the topic string to figure out if there's an object accessible from the global environment with that name. If there is one, it tries to acquire a help_handler by calling a new ark method called ark_help_get_handler. If a helper is found, it's called for its side effects of showing the documentation in the help pane.

  • The last change just adds an entrypoint to display a html file in the help pane. This is useful for showing help topipics that may be rendered by servers that are not the R Help server.

For instance, this how the method implementation looks like for reticulate:

.ark.register_method(
  "ark_positron_help_get_handler",
  "python.builtin.object",
  function(obj, ...) {
    if (is.null(.globals$pydoc_thread)) {
      .globals$pydoc_thread <- positron$pydoc$start_server(port = 64216L)
    }

    function(topic) {
      url <- paste0(
        .globals$pydoc_thread$url,
        "get?key=",
        positron$utils$get_qualname(obj)
      )

      .ps.help.browse_external_url(url)
      TRUE
    }
  }
)

@dfalbel dfalbel requested a review from Copilot April 29, 2025 14:08
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enables extensions to contribute to the F1 show help topic, enhancing the help system to support both object-based and external HTML documentation sources.

  • Adjust help topic node location detection to handle extract operator syntax.
  • Enhance showHelpTopic to evaluate the topic and dispatch to a dedicated help handler when appropriate.
  • Introduce a new entrypoint for displaying external HTML help content.

Reviewed Changes

Copilot reviewed 4 out of 6 changed files in this pull request and generated no comments.

File Description
crates/ark/src/lsp/help_topic.rs Added check for ExtractOperator in help node location logic.
crates/ark/src/help/r_help.rs Modified help URL handling to support different URL kinds; added new entrypoint.
crates/ark/src/help/message.rs Introduced ShowHelpUrlKind enum to differentiate URL handling.
crates/ark/src/browser.rs Updated help URL event construction with new URL kind.
Files not reviewed (2)
  • crates/ark/src/modules/positron/help.R: Language not supported
  • crates/ark/src/modules/positron/methods.R: Language not supported
Comments suppressed due to low confidence (2)

crates/ark/src/help/r_help.rs:214

  • [nitpick] The log message uses 'R url' and 'proxy url' labels, but when ShowHelpUrlKind is External both values are identical. Consider updating the log message to clarify the context or using more consistent naming to avoid confusion.
log::trace!("Sending frontend event `ShowHelp` with R url '{}' and proxy url '{}'", params.url, url);

crates/ark/src/help/message.rs:19

  • [nitpick] The variant 'HelpProxy' may be ambiguous; consider renaming it to a more descriptive name (e.g., 'Internal') to better reflect its intended purpose.
pub enum ShowHelpUrlKind {

@dfalbel dfalbel requested a review from lionel- April 29, 2025 16:53
@lionel-
Copy link
Contributor

lionel- commented May 1, 2025

This is a cool idea but I'm a bit uneasy about adding LSP features that require more dynamism.

showHelpTopic now evaluates the topic string to figure out if there's an object accessible from the global environment with that name. If there is one, it tries to acquire a help_handler by calling a new ark method called ark_help_get_handler

What if the object is inside a function or local environment? It sounds like this is the sort of feature that would be hard (probably not impossible though) to support lexically.

Features that only work at top level sound a bit unappealing to me. That said top-level scripting is a big use case for scripting languages like R. So I guess that would be the justification. We could guard the feature and disable it for objects shadowed by local bindings.

The other issue is that features relying on dynamic inspection are dependent on the state of the repl. For instance:

foo <- foo()
foo <- bar()

What result you'll get for foo depends on whether you've evaluated 0, 1, or 2 lines. Which is not necessarily a deal breaker but is not the ideal experience.

@dfalbel
Copy link
Contributor Author

dfalbel commented May 1, 2025

I definitely agree that there are drawbacks on supporting top-level only and relying on the interpreter state.

Most R users are already used to it, though. They expect completions for data.frame column names, for list components, R6 object methods and so on. I think a lot of programmers end up chosing the argument names of functions to match some object in the top level, so they have completions enabled. I'd think users also expect help to work similarly.

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 this pull request may close these issues.

2 participants