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

feat: More ways to specify R path #64

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

Conversation

gadenbuie
Copy link
Collaborator

Fixes #61

Adds support for consulting the IDE for the preferred R runtime in two ways:

  1. In Positron, we now consult the preffered runtime, i.e. the runtime selected by the user for the project. The extension will follow the prefferred runtime version on launch, meaning that if you launch the app with R 4.4, change to R 4.2, and relaunch the app, the second launch will use R 4.2.

  2. In VS Code, the R Debugger extension includes r.rpath.{windows,mac,linux} settings. We also consult this setting. Note that the R Debugger extension should not be used in Positron. We don't skip this check if in Positron (maybe you're syncing settings?), though.

The R Path consultation order is now:

  1. Positron
  2. r.rpath.{windows,mac,linux} setting
  3. $PATH
  4. Windows registry

@gadenbuie gadenbuie requested a review from jcheng5 July 8, 2024 15:46
if (!("acquirePositronApi" in globalThis)) { return; }
if (typeof globalThis.acquirePositronApi !== "function") { return; }

return globalThis.acquirePositronApi();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just out of curiosity, not asking for a change... Is this all equivalent to globalThis.acquirePositronApi?()? (Other than you're gracefully dealing with globalThis not being an object and acquirePositronApi not being a function, both of which seem unlikely?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You mean globalThis?.acquirePositronApi(), right? Then yes, it's equivalent if overly cautious. I'm happy to switch to the cleaner syntax.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm, I don't think that's right... I think globalThis?. checks for globalThis being null/undefined but not acquirePositronApi. It looks to me like ?() is not a thing, I think maybe the original is best.

Or

const apa = globalThis?.acquirePositronApi;
return apa ? apa() : undefined;

Or

if (typeof acquirePositronApi === "function") {
  return acquirePositronApi();
}

(Bikeshedding so hard right now... sorry... 😬)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or

globalThis?.acquirePositronApi?.call(globalThis)

(Alright I'm done, I promise!)

Copy link
Collaborator Author

@gadenbuie gadenbuie Jul 22, 2024

Choose a reason for hiding this comment

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

I don't think that's right... I think globalThis?. checks for globalThis being null/undefined but not acquirePositronApi

Oh yeah, that's right. I think this is pretty succinct and brings in the right ideas:

function getPositronAPI(): undefined | any {
    if (typeof globalThis?.acquirePositronApi !== "function") { return; }

    return globalThis.acquirePositronApi();
}

Copy link
Collaborator

@jcheng5 jcheng5 left a comment

Choose a reason for hiding this comment

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

Other than the one type tweak, LGTM!

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.

Define path to R executable
2 participants