Skip to content
This repository has been archived by the owner on Dec 23, 2021. It is now read-only.

Venv fixes #228

Merged
merged 7 commits into from
Feb 27, 2020
Merged

Venv fixes #228

merged 7 commits into from
Feb 27, 2020

Conversation

andreamah
Copy link
Contributor

Description:

User flow fixes, organization, and asks users if they want install globally if they refuse to do it in a venv

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Limitations:

Testing:

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Test A
  • Test B

Checklist:

  • My code follows the style guidelines of this project
  • My code has been formatted with npm run format and passes the checks in npm run check
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

Comment on lines +601 to +605
.showInformationMessage(
CONSTANTS.INFO.ARE_YOU_SURE,
DialogResponses.INSTALL_NOW,
DialogResponses.DONT_INSTALL
)

Choose a reason for hiding this comment

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

I think it would be ok to remove this prompt, as it is the third prompt in a row asking about installing dependencies. Up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

perhaps we should leave it in for now, since people who just close popups on instinct should be advised that the extension won't work without installing dependencies! Maybe we can incorporate this info into an earlier popup later on :0

await installDependenciesWrapper(
context,
pythonExecutableName
);

Choose a reason for hiding this comment

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

If you don't install the dependencies (perhaps you miss the prompts) and try running some micro:bit code, VS Code only shows a generic error:

[ERROR] Traceback (most recent call last):
  File "c:\Users\user\.vscode\extensions\vscode-python-devicesimulator\out\process_user_code.py", line 26, in <module>
    from common.telemetry import telemetry_py
  File "c:\Users\user\.vscode\extensions\vscode-python-devicesimulator\out\common\telemetry.py", line 1, in <module>
    from applicationinsights import TelemetryClient
ModuleNotFoundError: No module named 'applicationinsights'

It would be much more helpful if we could show the user a more helpful message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True! Perhaps that'd be good for another release, since we're rushing for the release in an hour :'''(

Copy link

@nasadigital nasadigital left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the fix 🛫 🌕

Copy link

@nasadigital nasadigital left a comment

Choose a reason for hiding this comment

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

It would also be very useful to add some telemetry to the prompts and see where users end up installing the dependencies to make data driven decisions in the future.

@vandyliu vandyliu merged commit 3d21ebb into dev Feb 27, 2020
@vandyliu vandyliu deleted the users/t-anmah/ext-venv-fix branch February 27, 2020 22:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants