Skip to content

Add Registry Locator#14349

Merged
karthiknadig merged 2 commits into
microsoft:mainfrom
karthiknadig:reg2
Oct 13, 2020
Merged

Add Registry Locator#14349
karthiknadig merged 2 commits into
microsoft:mainfrom
karthiknadig:reg2

Conversation

@karthiknadig
Copy link
Copy Markdown
Member

No description provided.

@karthiknadig karthiknadig added the no-changelog No news entry required label Oct 9, 2020
@codecov-io
Copy link
Copy Markdown

codecov-io commented Oct 12, 2020

Codecov Report

Merging #14349 into main will decrease coverage by 0.41%.
The diff coverage is 86.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #14349      +/-   ##
==========================================
- Coverage   59.81%   59.40%   -0.42%     
==========================================
  Files         709      716       +7     
  Lines       39414    39999     +585     
  Branches     5712     5793      +81     
==========================================
+ Hits        23577    23762     +185     
- Misses      14598    14976     +378     
- Partials     1239     1261      +22     
Impacted Files Coverage Δ
...c/client/pythonEnvironments/common/windowsUtils.ts 17.85% <40.00%> (ø)
...covery/locators/services/windowsRegistryLocator.ts 93.75% <92.30%> (ø)
src/client/pythonEnvironments/base/info/index.ts 100.00% <100.00%> (ø)
...nt/datascience/notebookStorage/vscNotebookModel.ts 20.23% <0.00%> (-37.66%) ⬇️
src/client/datascience/notebook/contentProvider.ts 44.64% <0.00%> (-6.52%) ⬇️
src/client/common/utils/platform.ts 68.00% <0.00%> (-4.00%) ⬇️
src/client/activation/extensionSurvey.ts 95.38% <0.00%> (-3.01%) ⬇️
...c/client/application/misc/joinMailingListPrompt.ts 97.56% <0.00%> (-2.44%) ⬇️
...cience/jupyter/jupyterCellOutputMimeTypeTracker.ts 74.24% <0.00%> (-1.95%) ⬇️
...client/datascience/kernel-launcher/kernelFinder.ts 72.91% <0.00%> (-1.95%) ⬇️
... and 51 more

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 a4d1667...6ddbbe1. Read the comment docs.

Copy link
Copy Markdown

@kimadeline kimadeline 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. Not going to block on this, but could you fix the [spaces](https://github.com/airbnb/javascript#whitespace--before-blocks and spaces) before merging? Thanks!

searchLocation?: Uri;
};

export const UNKNOWN_PYTHON_VERSION:PythonVersion = {
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 specific to this PR, but here you introduce the UNKNOWN_PYTHON_VERSION constant, then in src/client/common/utils/version.ts we have

export const EMPTY_VERSION: RawBasicVersionInfo = {
major: -1,
minor: -1,
micro: -1,
unnormalized: {
major: undefined,
minor: undefined,
micro: undefined
}
};

and in src/client/pythonEnvironments/base/info/env.ts we have buildEnvInfo that builds an empty env object, with almost the same field data.

It would be nice if we could have all these constants living together somewhere 😐

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.

We need to bring all of these together. Likely we will need to do a final polish when we are done with the functional bits.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is it too early to create a work item for this, or do you think we'll remember to do this when we're done?

try {
version = parseVersion(versionStr);
} catch (ex) {
version = {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why not use UNKNOWN_PYTHON_VERSION here?

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.

will do

@sonarqubecloud
Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@karthiknadig karthiknadig merged commit 1370b6d into microsoft:main Oct 13, 2020
@karthiknadig karthiknadig deleted the reg2 branch October 23, 2020 05:51
luabud pushed a commit to luabud/vscode-python that referenced this pull request Oct 26, 2020
* Add Registry Locator

* Address comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog No news entry required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants