More code reviews

Author: Kartik Raj

src/client/pythonEnvironments/common/externalDependencies.ts

@@ -4,7 +4,6 @@
 import * as fsapi from 'fs-extra';
 import * as path from 'path';
 import { ExecutionResult, IProcessServiceFactory } from '../../common/process/types';
-import { createDeferred } from '../../common/utils/async';
 import { getOSType, OSType } from '../../common/utils/platform';
 import { IServiceContainer } from '../../ioc/types';
 
@@ -23,11 +22,7 @@ export async function shellExecute(command: string, timeout: number): Promise<Ex
 }
 
 export function pathExists(absPath: string): Promise<boolean> {
-    const deferred = createDeferred<boolean>();
-    fsapi.exists(absPath, (result) => {
-        deferred.resolve(result);
-    });
-    return deferred.promise;
+    return fsapi.pathExists(absPath);
 }
 
 export function readFile(filePath: string): Promise<string> {

src/client/pythonEnvironments/discovery/locators/services/pipEnvHelper.ts

@@ -6,20 +6,20 @@ import { traceError } from '../../../../common/logger';
 import { getEnvironmentVariable } from '../../../../common/utils/platform';
 import { arePathsSame, pathExists, readFile } from '../../../common/externalDependencies';
 
-function getSearchDepth() {
+function getSearchHeight() {
     // PIPENV_MAX_DEPTH tells pipenv the maximum number of directories to recursively search for
     // a Pipfile, defaults to 3: https://pipenv.pypa.io/en/latest/advanced/#pipenv.environments.PIPENV_MAX_DEPTH
     const maxDepthStr = getEnvironmentVariable('PIPENV_MAX_DEPTH');
-    if (maxDepthStr !== undefined) {
-        const maxDepth = parseInt(maxDepthStr, 10);
-        // eslint-disable-next-line no-restricted-globals
-        if (isNaN(maxDepth)) {
-            traceError(`PIPENV_MAX_DEPTH is incorrectly set. Converting value '${maxDepthStr}' to number results in NaN`);
-            return 1;
-        }
-        return maxDepth;
+    if (maxDepthStr === undefined) {
+        return 3;
+    }
+    const maxDepth = parseInt(maxDepthStr, 10);
+    // eslint-disable-next-line no-restricted-globals
+    if (isNaN(maxDepth)) {
+        traceError(`PIPENV_MAX_DEPTH is incorrectly set. Converting value '${maxDepthStr}' to number results in NaN`);
+        return 1;
     }
-    return 3;
+    return maxDepth;
 }
 
 /**
@@ -32,15 +32,15 @@ export async function _getAssociatedPipfile(
     options: {lookIntoParentDirectories: boolean},
 ): Promise<string | undefined> {
     const pipFileName = getEnvironmentVariable('PIPENV_PIPFILE') || 'Pipfile';
-    let depthToSearch = options.lookIntoParentDirectories ? getSearchDepth() : 1;
-    while (depthToSearch > 0 && !arePathsSame(searchDir, path.dirname(searchDir))) {
+    let heightToSearch = options.lookIntoParentDirectories ? getSearchHeight() : 1;
+    while (heightToSearch > 0 && !arePathsSame(searchDir, path.dirname(searchDir))) {
         const pipFile = path.join(searchDir, pipFileName);
         // eslint-disable-next-line no-await-in-loop
         if (await pathExists(pipFile)) {
             return pipFile;
         }
         searchDir = path.dirname(searchDir);
-        depthToSearch -= 1;
+        heightToSearch -= 1;
     }
     return undefined;
 }
@@ -97,8 +97,8 @@ async function getProjectDir(envFolder: string): Promise<string | undefined> {
  */
 async function getPipfileIfGlobal(interpreterPath: string): Promise<string | undefined> {
     const envFolder = path.dirname(path.dirname(interpreterPath));
-    const project = await getProjectDir(envFolder);
-    if (project === undefined) {
+    const projectDir = await getProjectDir(envFolder);
+    if (projectDir === undefined) {
         return undefined;
     }
 
@@ -107,11 +107,11 @@ async function getPipfileIfGlobal(interpreterPath: string): Promise<string | und
     // |__ Pipfile  <--- check if Pipfile exists here and return it
     // The name of the project (directory where Pipfile resides) is used as a prefix in the environment folder
     const envFolderName = path.basename(envFolder);
-    if (!envFolderName.startsWith(`${path.basename(project)}-`)) {
+    if (!envFolderName.match(new RegExp(`^${path.basename(projectDir)}-[0-9a-fA-F]+$`)) === null) {
         return undefined;
     }
 
-    return _getAssociatedPipfile(project, { lookIntoParentDirectories: false });
+    return _getAssociatedPipfile(projectDir, { lookIntoParentDirectories: false });
 }
 
 /**

src/test/pythonEnvironments/discovery/locators/pipEnvHelper.functional.test.ts

@@ -26,7 +26,9 @@ suite('Pipenv utils', () => {
         readFile.withArgs(expectedDotProjectFile).resolves(project);
         const interpreterPath: string = path.join(TEST_LAYOUT_ROOT, 'pipenv', 'globalEnvironments', 'project3-2s1eXEJ2', 'Scripts', 'python.exe');
         const folder = path.join(project, 'parent', 'child', 'folder');
+
         const isRelated = await isPipenvEnvironmentRelatedToFolder(interpreterPath, folder);
+
         assert.equal(isRelated, true);
     });
 });

src/test/pythonEnvironments/discovery/locators/pipEnvHelper.unit.test.ts

@@ -1,12 +1,14 @@
 import * as assert from 'assert';
-import * as path from 'path';
+import * as pathModule from 'path';
 import * as sinon from 'sinon';
 import * as platformApis from '../../../../client/common/utils/platform';
 import * as externalDependencies from '../../../../client/pythonEnvironments/common/externalDependencies';
 import { _getAssociatedPipfile, isPipenvEnvironment, isPipenvEnvironmentRelatedToFolder } from '../../../../client/pythonEnvironments/discovery/locators/services/pipEnvHelper';
 
+const path = platformApis.getOSType() === platformApis.OSType.Windows ? pathModule.win32 : pathModule.posix;
+
 suite('Pipenv helper', () => {
-    suite('Method isPipenvEnvironmentRelatedToFolder()', async () => {
+    suite('isPipenvEnvironmentRelatedToFolder()', async () => {
         let readFile: sinon.SinonStub;
         let getEnvVar: sinon.SinonStub;
         let pathExists: sinon.SinonStub;
@@ -74,7 +76,6 @@ suite('Pipenv helper', () => {
             const pipFileAssociatedWithFolder = path.join(folder, 'Pipfile');
             // Pipfile associated with folder exists
             pathExists.withArgs(pipFileAssociatedWithFolder).resolves(true);
-
             // But the paths to both Pipfiles aren't the same
             arePathsSame.withArgs(pipFileAssociatedWithEnvironment, pipFileAssociatedWithFolder).resolves(false);
 
@@ -107,7 +108,7 @@ suite('Pipenv helper', () => {
         });
     });
 
-    suite('Method isPipenvEnvironment()', async () => {
+    suite('isPipenvEnvironment()', async () => {
         let readFile: sinon.SinonStub;
         let getEnvVar: sinon.SinonStub;
         let pathExists: sinon.SinonStub;
@@ -203,7 +204,7 @@ suite('Pipenv helper', () => {
         });
     });
 
-    suite('Method _getAssociatedPipfile()', async () => {
+    suite('_getAssociatedPipfile()', async () => {
         let getEnvVar: sinon.SinonStub;
         let pathExists: sinon.SinonStub;
         setup(() => {