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

Update agent methods to use the selected R interpreter consistently #2501

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

sagerb
Copy link
Collaborator

@sagerb sagerb commented Dec 17, 2024

Intent

The changes within this PR are designed to implement a consistent usage of the active version of R, when running within Positron. The changes apply to VS Code as well, however, until we implement some of the other issues within the associated epic to gather configured versions, Positron will be the place which exposes the functionality the cleanest.

The associated issue outlines this logic change for determining the active R version to use:

  1. Use the passed-in path for the R interpreter if supplied. It must exist and be validated)
    • If it doesn't exist, fall through to the next option
    • If it does exist but is not a valid R interpreter (use --version output to validate), then return an error
  2. Use the first R executable found on PATH; it must exist and be validated.
    • If not valid or doesn't exist, return an error

Once the interpreter is identified, it should be used for creating/retrieving the lock file, as well as when running commands.

Resolves #2500

Type of Change

    • Bug Fix
    • New Feature
    • Breaking Change
    • Documentation
    • Refactor
    • Tooling

Approach

The existing codebase needed to be more consistent in determining which path was to be used when invoking the R interpreter. There was no clear, specific location where the logic associated with the determination algorithm was isolated. As this task (which is part of the epic hUse the active versions of interpreters instead of the first version on PATH) required changes to this logic, a refactor seemed the best approach to get consistent behavior in place and have a logical location to modify it if we needed to.

The main refactoring involved with this change involved removing the R interpreter-related functionality from the inspect package and adding it to a newly created package, interpreters. I intend to do the same type of refactoring for the other interpreters within this epic (Python and Quato), so this package will be expanded upon soon.

Once the existing code was refactored, it was updated to implement the logic listed in the issue (#2500).

In addition, the functionality of determining if there is a dependency on R was moved within the interpreters package. It had previously lived within the initialization code, but that appeared to have been implemented as a "guard" before usage of the inspect package. Once the changes were made, I moved the requiresR method into the inspect package.

With this in place, all other usages within the code base for an R interpreter were refactored to use the new interpreters package. The need for these other areas of the codebase to access this functionality was a major factor in why I split the functionality out into a separate package. It just didn't seem "right" to have the other areas depend upon the inspect package when they simply wanted an interpreter.

Many unit tests were either moved or updated to work with the new package. I've been careful to maintain the intention of the previous tests and add new ones.

User Impact

The concept we're trying to maintain here is that when a user has set up their environment within an IDE for a particular version of R, they expect that the same version will be used for all operations of the publisher extension. While we have not yet implemented all of the hooks to gather the "active" version of R, the API requests are in place so that we can set that value from the extension code.

While many of the other changes will not be visible to the user, a user will see the consistent behavior when creating a deployment for a project; the configuration file created will have an R section which properly reflects the new approach.

Automated Tests

Directions for Reviewers

Unfortunately, this PR covers more changes than we usually try to have within a single PR. Unfortunately, the impact to the unit tests greatly exploded the changes required. I'd suggest looking at the changes by first examining the main functionality which was refactored. This can be seen by viewing the changes to inspect/r.go as functionality was moved into interpreters/r.go. Changes to the other files fall into two buckets:

  1. calling this new functionality to get the active R interpreter
  2. unit test updates

I recommend using Positron for the validation process, with a configuration that includes two or more versions of R on the system. With an existing R project open, you should be able to create a deployment for a project and have its configuration file populated with an R section reflecting the values of the selected version. Switching the version via Positron and then creating a new deployment should produce a configuration that contains an R section reflecting the active version selected.

Checklist

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.

Implement rExecutable discovery to prefer the active version of R within the IDE rather than path
1 participant