Skip to content

Activate extension if workspace has mspythonconfig #10906

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 14, 2020
Merged

Activate extension if workspace has mspythonconfig #10906

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,8 @@
"onCommand:python.datascience.selectJupyterInterpreter",
"onCommand:python.datascience.selectjupytercommandline",
"onCommand:python.enableSourceMapSupport",
"onCustomEditor:NativeEditorProvider.ipynb"
"onCustomEditor:NativeEditorProvider.ipynb",
"workspaceContains:**/mspythonconfig.json"
Copy link

Choose a reason for hiding this comment

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

If JSON supported comments I'd ask you to leave one here about the purpose of this activation trigger... 😄 In lieu of that, would you mind making at note somewhere else (e.g. in CONTRIBUTING.md, src/client/extension.ts, or perhaps some new file like PACKAGE_JSON_COMMENTS.md)?

Copy link

Choose a reason for hiding this comment

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

Not a fan of the prefix ms. Why not pythonconfig.json?
The launch.json and similar files are specific to VSC (Microsoft product), yet we dont prefix with ms..

Copy link

Choose a reason for hiding this comment

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

However, i understand this is some existing file from another product/package.

Copy link

Choose a reason for hiding this comment

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

@MikhailArkhipov was there a good reason for the ms prefix? Can we remove it?

Copy link
Member Author

Choose a reason for hiding this comment

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

pythonconfig.json is confusing, it makes me believe this is to configure Python while using the extension. mspythonconfig.json sounds more likely to do with the extension, still not clear enough.

Copy link

Choose a reason for hiding this comment

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

The extension is named ms-python.python-nnnn
So the ms prefix is consistent.

Also, pythonconfig.json is too generic and might conflict with some other extension. Does that make sense?

Copy link

@DonJayamanne DonJayamanne Apr 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Still not sold. Feels more like branding when sticking ms.
If we want a config for the extension in VS Code then how about, codepythonconfig.json, or vss/vscode/pvsc, or similar.

And we might want to put it into ./.vscode folder along with settings.json and launch.json. This way there's no ambiguity about who owns this file.

If it goes into ./.vscode folder I don't see any reason for not naming it pythonconfig.json. At that point it is obvious that it is specific to vscode, automatically our extension.

Copy link
Member

Choose a reason for hiding this comment

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

This file is read directly by the LS, so I don't think it should be in any editor-specific folder.

],
"main": "./out/client/extension",
"contributes": {
Expand Down