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

Detect ActiveState Python runtimes #20534

Merged
merged 24 commits into from
Feb 22, 2023
Merged

Conversation

mitchell-as
Copy link

@mitchell-as mitchell-as commented Jan 19, 2023

Closes #20532

@mitchell-as
Copy link
Author

mitchell-as commented Jan 20, 2023 via email

Copy link

@karrtikr karrtikr left a comment

Choose a reason for hiding this comment

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

Thanks for the comprehensive PR, great work! Almost there.

@karrtikr karrtikr added the feature-request Request for new features or functionality label Jan 23, 2023
@karrtikr karrtikr changed the title Detect ActiveState Python runtimes (#20532) Detect ActiveState Python runtimes Jan 23, 2023
Copy link

@karrtikr karrtikr left a comment

Choose a reason for hiding this comment

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

Follow up: Is it there a cheaper way to check whether ActiveState is installed, rather than running the binary? Perhaps check a standard file location first, or a registry key.

The concern is to avoid running this for every session, if the probability of us finding it is less.

@mitchell-as
Copy link
Author

Follow up: Is it there a cheaper way to check whether ActiveState is installed, rather than running the binary? Perhaps check a standard file location first, or a registry key.

The concern is to avoid running this for every session, if the probability of us finding it is less.

Yes, I have committed changes to look for an installation of our tool on the local filesystem. If there is no installation, then no attempt will be made to detect our runtimes (and thus no shell execute).

@mitchell-as
Copy link
Author

One more addition: in the event that the user would like to specify the path to our tool on the local filesystem, I've added a preference to do so.

@mitchell-as
Copy link
Author

Thanks for all of your time and feedback so far. I really appreciate it.

karrtikr
karrtikr previously approved these changes Feb 2, 2023
Copy link

@karrtikr karrtikr left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for working through all the comments, feel free to let us know if you need anything more.

karthiknadig
karthiknadig previously approved these changes Feb 2, 2023
@mitchell-as mitchell-as requested review from brettcannon and karrtikr and removed request for brettcannon and karrtikr February 7, 2023 18:52
@karrtikr
Copy link

karrtikr commented Feb 7, 2023

Lint appears to be failing, this will need to be updated using main branch as well.

@mitchell-as
Copy link
Author

Lint seems to be fixed now.

@karrtikr karrtikr added the skip package*.json package.json and package-lock.json don't both need updating label Feb 8, 2023
@karthiknadig karthiknadig enabled auto-merge (squash) February 22, 2023 00:08
@karthiknadig karthiknadig merged commit c18e8c9 into microsoft:main Feb 22, 2023
wesm pushed a commit to posit-dev/positron that referenced this pull request Mar 28, 2024
--------------------
Commit message for microsoft/vscode-python@fc72be9:

Show `Python: Report issue` command in palette regardless of whether a Python file is opened (microsoft/vscode-python#20726)

Closes microsoft/vscode-python#20723
--------------------
Commit message for microsoft/vscode-python@c18e8c9:

Detect ActiveState Python runtimes (microsoft/vscode-python#20534)

Closes microsoft/vscode-python#20532
--------------------
Commit message for microsoft/vscode-python@2152cd9:

Don't set `formatOnType` for auto-indent experiment if it's already set (microsoft/vscode-python#20710)


--------------------
Commit message for microsoft/vscode-python@995b0bc:

Add support for 'back' to all create env UI. (microsoft/vscode-python#20693)

Closes microsoft/vscode-python#20274


### Usage

This change allows callers of the Create Environment command to handle
`Back` and `Cancel`:
``` typescript
let result: CreateEnvironmentResult | undefined;
try {
    const result = await commands.executeCommand("python.createEnvironment", {showBackButton: true});
} catch(e) {
   // error while creating environment
}

if (result?.action === 'Back') {
    // user clicked Back
}

if (result?.action === 'Cancel') {
    // user pressed escape or Cancel
}
```
I decided to go with `result?.action` because we don't have a npm
package for python extension API so catching particular exception might
be error prone with `ex instanceof <error>`. We will provide a proper
interface via `api.environments` for create environment, and
contribution to create environment. Until that point this command will
provide the stop gap.

### Notes

1. I did not use the multi-step input that is used in the rest of the
extension because, the existing implementation does not have context.
Consider the following scenario: venv -> workspace select -> python
select -> packages. Assume that there is only one workspace, and we
don't show the workspace selection UI, that decision is done inside the
workspace step. So, if there is only 1 workspace it is a short circuit
to next step. User is on python selection and clicks `back`, workspace
selection short circuits to next step which is python selection. So,
from user perspective, back does not work. This can be fixed by sending
context that the reason control moved to previous step was because user
clicked on back.
2. This makes a change to old multi step API to rethrow the exception,
if user hits `back` and the current step has no steps to go back to.
--------------------
Commit message for microsoft/vscode-python@f3ecbf5:

Fix-conda-version-parsing (microsoft/vscode-python#20674)


--------------------
Commit message for microsoft/vscode-python@a6a6f50:

Inactive pytest run command (microsoft/vscode-python#20653)

Here the new flow is created but kept inactive for the pytest execution
--------------------
Commit message for microsoft/vscode-python@2202fbe:

Call the correct API to determine if a user is in treatment or control group (microsoft/vscode-python#20690)

Closes microsoft/vscode-python#20183
--------------------
Commit message for microsoft/vscode-python@b0ab10d:

Only use activated environment from terminal if VSCode was launched via CLI (microsoft/vscode-python#20667)

Closes microsoft/vscode-python#20644
--------------------
Commit message for microsoft/vscode-python@02a92fc:

Ensure interpreter path isn't truncated for workspace-relative paths when storing value (microsoft/vscode-python#20661)

For microsoft/vscode-python#20660

I'm not quite sure why this was done. It doesn't make sense to do this
only for display.
--------------------
Commit message for microsoft/vscode-python@377067f:

Use correct API to get interpreter path for language servers (microsoft/vscode-python#20656)

For microsoft/vscode-python#20644 closes
microsoft/vscode-python#20657
--------------------
Commit message for microsoft/vscode-python@cd6ca9d:

Remove `isort` extension dependency (microsoft/vscode-python#20577)

Closes microsoft/vscode-python#20586

Lead-authored-by: Kartik Raj <karraj@microsoft.com>
Co-authored-by: Erik De Bonte <erikd@microsoft.com>
Co-authored-by: Karthik Nadig <kanadig@microsoft.com>
Co-authored-by: mitchell <mitchellb@activestate.com>
Co-authored-by: Pete Farland <pete.farland@posit.co>
Co-authored-by: Eleanor Boyd <eleanorboyd@microsoft.com>
wesm pushed a commit to posit-dev/positron that referenced this pull request Mar 28, 2024
--------------------
Commit message for microsoft/vscode-python@fc72be9:

Show `Python: Report issue` command in palette regardless of whether a Python file is opened (microsoft/vscode-python#20726)

Closes microsoft/vscode-python#20723
--------------------
Commit message for microsoft/vscode-python@c18e8c9:

Detect ActiveState Python runtimes (microsoft/vscode-python#20534)

Closes microsoft/vscode-python#20532
--------------------
Commit message for microsoft/vscode-python@2152cd9:

Don't set `formatOnType` for auto-indent experiment if it's already set (microsoft/vscode-python#20710)


--------------------
Commit message for microsoft/vscode-python@995b0bc:

Add support for 'back' to all create env UI. (microsoft/vscode-python#20693)

Closes microsoft/vscode-python#20274


### Usage

This change allows callers of the Create Environment command to handle
`Back` and `Cancel`:
``` typescript
let result: CreateEnvironmentResult | undefined;
try {
    const result = await commands.executeCommand("python.createEnvironment", {showBackButton: true});
} catch(e) {
   // error while creating environment
}

if (result?.action === 'Back') {
    // user clicked Back
}

if (result?.action === 'Cancel') {
    // user pressed escape or Cancel
}
```
I decided to go with `result?.action` because we don't have a npm
package for python extension API so catching particular exception might
be error prone with `ex instanceof <error>`. We will provide a proper
interface via `api.environments` for create environment, and
contribution to create environment. Until that point this command will
provide the stop gap.

### Notes

1. I did not use the multi-step input that is used in the rest of the
extension because, the existing implementation does not have context.
Consider the following scenario: venv -> workspace select -> python
select -> packages. Assume that there is only one workspace, and we
don't show the workspace selection UI, that decision is done inside the
workspace step. So, if there is only 1 workspace it is a short circuit
to next step. User is on python selection and clicks `back`, workspace
selection short circuits to next step which is python selection. So,
from user perspective, back does not work. This can be fixed by sending
context that the reason control moved to previous step was because user
clicked on back.
2. This makes a change to old multi step API to rethrow the exception,
if user hits `back` and the current step has no steps to go back to.
--------------------
Commit message for microsoft/vscode-python@f3ecbf5:

Fix-conda-version-parsing (microsoft/vscode-python#20674)


--------------------
Commit message for microsoft/vscode-python@a6a6f50:

Inactive pytest run command (microsoft/vscode-python#20653)

Here the new flow is created but kept inactive for the pytest execution
--------------------
Commit message for microsoft/vscode-python@2202fbe:

Call the correct API to determine if a user is in treatment or control group (microsoft/vscode-python#20690)

Closes microsoft/vscode-python#20183
--------------------
Commit message for microsoft/vscode-python@b0ab10d:

Only use activated environment from terminal if VSCode was launched via CLI (microsoft/vscode-python#20667)

Closes microsoft/vscode-python#20644
--------------------
Commit message for microsoft/vscode-python@02a92fc:

Ensure interpreter path isn't truncated for workspace-relative paths when storing value (microsoft/vscode-python#20661)

For microsoft/vscode-python#20660

I'm not quite sure why this was done. It doesn't make sense to do this
only for display.
--------------------
Commit message for microsoft/vscode-python@377067f:

Use correct API to get interpreter path for language servers (microsoft/vscode-python#20656)

For microsoft/vscode-python#20644 closes
microsoft/vscode-python#20657
--------------------
Commit message for microsoft/vscode-python@cd6ca9d:

Remove `isort` extension dependency (microsoft/vscode-python#20577)

Closes microsoft/vscode-python#20586

Lead-authored-by: Kartik Raj <karraj@microsoft.com>
Co-authored-by: Erik De Bonte <erikd@microsoft.com>
Co-authored-by: Karthik Nadig <kanadig@microsoft.com>
Co-authored-by: mitchell <mitchellb@activestate.com>
Co-authored-by: Pete Farland <pete.farland@posit.co>
Co-authored-by: Eleanor Boyd <eleanorboyd@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for new features or functionality skip package*.json package.json and package-lock.json don't both need updating
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detect ActiveState Python Runtimes
5 participants