Activate extension if workspace has mspythonconfig by karthiknadig · Pull Request #10906 · microsoft/vscode-python · GitHub
Skip to content

Activate extension if workspace has mspythonconfig#10906

Merged
karthiknadig merged 1 commit into
microsoft:masterfrom
karthiknadig:task1061411
Apr 14, 2020
Merged

Activate extension if workspace has mspythonconfig#10906
karthiknadig merged 1 commit into
microsoft:masterfrom
karthiknadig:task1061411

Conversation

@karthiknadig

Copy link
Copy Markdown
Member

No description provided.

@karthiknadig karthiknadig added the no-changelog No news entry required label Apr 1, 2020
@sonarqubecloud

sonarqubecloud Bot commented Apr 1, 2020

Copy link
Copy Markdown

@codecov-io

Copy link
Copy Markdown

Codecov Report

Merging #10906 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10906      +/-   ##
==========================================
- Coverage   60.67%   60.65%   -0.02%     
==========================================
  Files         580      580              
  Lines       31530    31530              
  Branches     4480     4480              
==========================================
- Hits        19131    19125       -6     
- Misses      11429    11433       +4     
- Partials      970      972       +2     
Impacted Files Coverage Δ
src/datascience-ui/react-common/arePathsSame.ts 75.00% <0.00%> (-12.50%) ⬇️
src/client/common/utils/platform.ts 64.70% <0.00%> (-11.77%) ⬇️
src/client/linters/pydocstyle.ts 86.66% <0.00%> (-2.23%) ⬇️
src/client/datascience/debugLocationTracker.ts 76.56% <0.00%> (-1.57%) ⬇️
src/client/common/process/proc.ts 14.49% <0.00%> (-0.73%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f774d9...61c1d5c. Read the comment docs.

@ericsnowcurrently ericsnowcurrently left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

Comment thread package.json
"onCommand:python.enableSourceMapSupport",
"onCustomEditor:NativeEditorProvider.ipynb"
"onCustomEditor:NativeEditorProvider.ipynb",
"workspaceContains:**/mspythonconfig.json"

Copy link
Copy Markdown

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
Copy Markdown

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
Copy Markdown

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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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?

@DonJayamanne DonJayamanne Apr 2, 2020

Copy link
Copy Markdown

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
Copy Markdown
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.

@DonJayamanne DonJayamanne left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Isn't this a dup of #10856

@ericsnowcurrently

Copy link
Copy Markdown

@DonJayamanne

Isn't this a dup of #10856

This PR is for a specific file (presumably needed by the language server) that is not part of that other PR.

@karthiknadig

Copy link
Copy Markdown
Member Author

@karthiknadig karthiknadig merged commit deaeb45 into microsoft:master Apr 14, 2020
@lock lock Bot locked as resolved and limited conversation to collaborators Apr 25, 2020
@karthiknadig karthiknadig deleted the task1061411 branch April 28, 2020 23:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

no-changelog No news entry required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants