Skip to content

Add pyenv locator#14587

Merged
karthiknadig merged 9 commits intomicrosoft:mainfrom
karthiknadig:pyenv1
Nov 4, 2020
Merged

Add pyenv locator#14587
karthiknadig merged 9 commits intomicrosoft:mainfrom
karthiknadig:pyenv1

Conversation

@karthiknadig
Copy link
Copy Markdown
Member

No description provided.

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

codecov-io commented Oct 31, 2020

Codecov Report

Merging #14587 into main will increase coverage by 0.17%.
The diff coverage is 89.30%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #14587      +/-   ##
==========================================
+ Coverage   64.90%   65.07%   +0.17%     
==========================================
  Files         541      541              
  Lines       25144    25377     +233     
  Branches     3545     3587      +42     
==========================================
+ Hits        16320    16515     +195     
- Misses       8162     8179      +17     
- Partials      662      683      +21     
Impacted Files Coverage Δ
...nts/base/locators/composite/environmentsReducer.ts 95.58% <ø> (ø)
...pythonEnvironments/common/environmentIdentifier.ts 96.77% <ø> (ø)
...ient/pythonEnvironments/base/info/pythonVersion.ts 76.47% <68.75%> (-3.53%) ⬇️
...rc/client/pythonEnvironments/common/commonUtils.ts 85.93% <81.25%> (-14.07%) ⬇️
.../locators/services/virtualEnvironmentIdentifier.ts 90.41% <86.84%> (-6.96%) ⬇️
.../pythonEnvironments/common/externalDependencies.ts 72.34% <90.00%> (+4.77%) ⬆️
...nments/discovery/locators/services/condaLocator.ts 93.93% <93.54%> (-6.07%) ⬇️
...nments/discovery/locators/services/pyenvLocator.ts 94.49% <95.87%> (+5.02%) ⬆️
src/client/pythonEnvironments/base/info/env.ts 80.39% <100.00%> (+2.39%) ⬆️
...discovery/locators/services/windowsStoreLocator.ts 86.79% <0.00%> (-13.21%) ⬇️
... and 8 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 2352196...14436b1. Read the comment docs.

Comment thread src/client/pythonEnvironments/base/info/env.ts Outdated
Comment thread src/client/pythonEnvironments/common/commonUtils.ts Outdated
Comment thread src/client/pythonEnvironments/common/commonUtils.ts Outdated
Comment thread src/client/pythonEnvironments/discovery/locators/services/condaLocator.ts Outdated
Comment thread src/client/pythonEnvironments/discovery/locators/services/condaLocator.ts Outdated
Comment thread src/client/pythonEnvironments/discovery/locators/services/condaLocator.ts Outdated
Comment thread src/client/pythonEnvironments/base/info/env.ts Outdated
Comment thread src/client/pythonEnvironments/common/commonUtils.ts Outdated
Comment thread src/client/pythonEnvironments/common/environmentIdentifier.ts Outdated
Comment thread src/client/pythonEnvironments/common/environmentIdentifier.ts Outdated
Comment thread src/client/pythonEnvironments/discovery/locators/services/condaLocator.ts Outdated
@karthiknadig karthiknadig added no-changelog No news entry required and removed no-changelog No news entry required labels Nov 3, 2020
@karthiknadig karthiknadig requested a review from karrtikr November 3, 2020 23:32
Copy link
Copy Markdown

@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.

Awesome work

let specificity = getPythonVersionSpecificity(version);
for (const v of pythonVersions) {
if (v) {
const curSpecificity = getPythonVersionSpecificity(v);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can you use comparePythonVersionSpecificity and not export getPythonVersionSpecificity at all?

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.

Ah missed this one.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

getPythonVersionSpecificity is still being exported.

Comment thread src/client/pythonEnvironments/discovery/locators/services/pyenvLocator.ts Outdated
Comment thread src/client/pythonEnvironments/discovery/locators/services/pyenvLocator.ts Outdated
Comment thread src/client/pythonEnvironments/discovery/locators/services/pyenvLocator.ts Outdated
.map((p) => allParsers.get(p))
.filter((p) => p !== undefined);

if (parsers.length > 0 && parsers[0]) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
if (parsers.length > 0 && parsers[0]) {
if (parsers.length > 0) {

Will this work?

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.

Nope. Even though we filter out undefined, TS thinks that parsers[0] can be undefined.

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 you use the .map as I suggested above you should be able to get rid of parsers[0] check.

Comment thread src/test/pythonEnvironments/discovery/locators/pyenvLocator.unit.test.ts Outdated
Comment thread src/test/pythonEnvironments/discovery/locators/pyenvLocator.unit.test.ts Outdated
@karthiknadig karthiknadig requested a review from karrtikr November 4, 2020 17:43
Copy link
Copy Markdown

@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.

LGTM otherwise

* Wn > Wn-1 + Wn-2 + ... W0
*/
function getPythonVersionInfoHeuristic(version: PythonVersion): number {
export function getPythonVersionSpecificity(version: PythonVersion): number {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This need not be exported.

const parsers = knownPrefixes
.filter((k) => str.startsWith(k))
.map((p) => allParsers.get(p))
.filter((p) => p !== undefined);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
.filter((p) => p !== undefined);
.filter((p) => p !== undefined);
.map((p) => p!);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@karthiknadig However small the comments are, I expect them to be resolved or replied to before merging, when I do approve.

.map((p) => allParsers.get(p))
.filter((p) => p !== undefined);

if (parsers.length > 0 && parsers[0]) {
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 you use the .map as I suggested above you should be able to get rid of parsers[0] check.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Nov 4, 2020

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 6074946 into microsoft:main Nov 4, 2020
@karthiknadig karthiknadig deleted the pyenv1 branch November 5, 2020 00:55
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