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

Change how to fetch pythonPath as we're deprecating python.pythonPath setting soon #604

Closed
karrtikr opened this issue Apr 20, 2020 · 15 comments

Comments

@karrtikr
Copy link

karrtikr commented Apr 20, 2020

Hi Python extension dev here👋

We will soon be deprecating python.pythonPath settings which you guys use here https://github.com/formulahendry/vscode-code-runner/blob/master/src/codeManager.ts#L363.

We're moving it out of settings to a VSCode storage. So you'll have to change the code to fetch interpreter path from a API we provide instead of fetching using settings.json, in a way that it's backwards compatible. The initial design of the API is listed here microsoft/vscode-python#11294

@karrtikr
Copy link
Author

karrtikr commented Apr 21, 2020

We've to make sure all these 4 cases work,

  1. Old version of Code Runner + Old version of the extension -> already works
  2. New version of Code Runner + Old version of the extension
  3. New version of Code Runner + New version of the extension
  4. Old version of Code Runner + New version of the extension

Proposed changes

Code runner

In the new version of code runner,

We would check if user is using the old or the new python extension using the new setting.

  • If he is using the old one, we would look for the settings in the old way
  • If he is using the new one, we would look for the settings in the new way using the API
  • If new version of code runner is being used, the above proposed changes work fine for cases 2 & 3.

Python extension

How to check from extension A if user is using the old version of extension B or the new one

Introduce a flag in package.json of both the extensions. Within extension A, check if package.json of extension B has flag set. If it is set, it's the newer version of the extension.

Python extension

  • Introduce a feature flag in package.json of the extension, which states if the extension is using the new interpreter storage or not. We're adding the following to our package.json ,
"featureFlags": {
    "usingNewInterpreterStorage": true
}

So you can access the flag using,

const extension = vscode.extensions.getExtension('ms-python.python')!;
const flagValue = extension.packageJSON.featureFlags.usingNewInterpreterStorage;

Code runner

Please add a similar flag in package.json and let us know. You can use the following to call our API, which makes sure the extension is activated before calling the API:

const extension = vscode.extensions.getExtension('ms-python.python')!;
if (!extension.isActive) {
    await extension.activate();
}
const pythonPath = extension.exports.settings.getExecutionCommand(resource).join(' ');

Note we will have to release Code runnner extension first, followed by the extension release. (which is targeted before May 12)

@formulahendry
Copy link
Owner

Thanks @karrtikr for your proposed changes. It looks great!
It seems the API is still in PR review.
Please let me know once it is ready, so that I could start to integrate the API in Code Runner.
It would also be helpful if you could kindly provide a simple code sample/snippet of calling the new API. 😄

@formulahendry
Copy link
Owner

formulahendry commented Apr 22, 2020

For the flag, I plan to use below name in Code Runner:

"featureFlags": {
    "usingNewPythonInterpreterPathApi": true
}

@karrtikr
Copy link
Author

karrtikr commented Apr 22, 2020

Hey @formulahendry 👋

Here's a sample code using debug portion of our API. We also need to make sure the extension is activated first before calling the API. So you can use the code below:

const extension = vscode.extensions.getExtension('ms-python.python')!;
if (!extension.isActive) {
    await extension.activate();
}
const pythonPath = extension.exports.settings.getExecutionCommand(resource).join(' ');

Also I realized that we should not put quotes around boolean true in feature flags, so you can use this instead

"featureFlags": {
    "usingNewPythonInterpreterPathApi": true
}

I've updated the Proposed changes comment to reflect these details as well. We're having a discussion on the design of the API, I'll let you know when we finalize it, cheers!

@karrtikr
Copy link
Author

karrtikr commented Apr 22, 2020

Check if we are using the old version of Code runner or the new one. If using the old one, show a prompt asking user to upgrade Code runner to new version to keep using it.

For case 4, to reduce the number of users that get this prompt. Can we only show the prompt to those who have the setting in code runner to use python.pythonPath?

It would be helpful if you could please provide a code snippet of how to accomplish that, which setting to check etc.

@karrtikr
Copy link
Author

We finalized the API, it's detailed in the issue description microsoft/vscode-python#11294. Please let me know if you've any questions.

@formulahendry
Copy link
Owner

Hi @karrtikr
Here is the code snippet:

const pythonExecutor = vscode.workspace.getConfiguration("code-runner", resource).get<string>("executorMap.python");
const isUsingPythonPath: boolean = pythonExecutor.includes("$pythonPath");

@formulahendry
Copy link
Owner

I see the PR is merged.
I remember the Python extension has Insider version. Pleased kindly let me know when it is available in Insider, then I could start working on the new API.

@karrtikr
Copy link
Author

Thanks for the snippet, will let you know.

@karrtikr
Copy link
Author

It's available in the Insiders now.

@karrtikr
Copy link
Author

@formulahendry Please let me know after you're able to make the release with the changes, after which I can merge microsoft/vscode-python#11395.

@formulahendry
Copy link
Owner

Sure! I will work on this and let you know!

@formulahendry
Copy link
Owner

Hi @karrtikr , I have just committed the changes.
Would you please have a try on the attached vsix file and see whether everything is expected from your side?
code-runner-0.9.18-py.zip

If the changes are OK for you, I will publish a new release.

@karrtikr
Copy link
Author

karrtikr commented May 1, 2020

Yep, all good from my side. I have a comment regarding your change 437456d#r38877025

Thanks again & let me know when it's available in the marketplace after which I'll merge my PR.

@formulahendry
Copy link
Owner

Hi @karrtikr , I have just published a new release. You could continue your work now. 😄
Closing this issue.

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

No branches or pull requests

2 participants