Skip to content

Commit

Permalink
Code reviews
Browse files Browse the repository at this point in the history
  • Loading branch information
Kartik Raj committed Sep 23, 2020
1 parent dab9ee5 commit 0963a07
Show file tree
Hide file tree
Showing 5 changed files with 39 additions and 38 deletions.
1 change: 1 addition & 0 deletions src/client/pythonEnvironments/base/info/interpreter.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ type ShellExecFunc = (command: string, timeout: number) => Promise<ShellExecResu

type Logger = {
info(msg: string): void;

error(msg: string): void;
};

Expand Down
2 changes: 1 addition & 1 deletion src/client/pythonEnvironments/base/locator.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ export type PythonLocatorQuery = BasicPythonLocatorQuery & {
searchLocations?: Uri[];
};

export type QueryForEvent<E> = E extends PythonEnvsChangedEvent ? PythonLocatorQuery : BasicPythonLocatorQuery;
type QueryForEvent<E> = E extends PythonEnvsChangedEvent ? PythonLocatorQuery : BasicPythonLocatorQuery;

/**
* A single Python environment locator.
Expand Down
4 changes: 2 additions & 2 deletions src/client/pythonEnvironments/collection/environmentsReducer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { traceVerbose } from '../../common/logger';
import { createDeferred } from '../../common/utils/async';
import { areSameEnvironment, PythonEnvInfo, PythonEnvKind } from '../base/info';
import {
ILocator, IPythonEnvsIterator, PythonEnvUpdatedEvent, QueryForEvent,
ILocator, IPythonEnvsIterator, PythonEnvUpdatedEvent, PythonLocatorQuery,
} from '../base/locator';
import { PythonEnvsChangedEvent } from '../base/watcher';

Expand Down Expand Up @@ -47,7 +47,7 @@ export class PythonEnvsReducer implements ILocator {
return this.parentLocator.resolveEnv(environment);
}

public iterEnvs(query?: QueryForEvent<PythonEnvsChangedEvent>): IPythonEnvsIterator {
public iterEnvs(query?: PythonLocatorQuery): IPythonEnvsIterator {
const didUpdate = new EventEmitter<PythonEnvUpdatedEvent | null>();
const incomingIterator = this.parentLocator.iterEnvs(query);
const iterator: IPythonEnvsIterator = iterEnvsIterator(incomingIterator, didUpdate);
Expand Down
12 changes: 6 additions & 6 deletions src/client/pythonEnvironments/collection/environmentsResolver.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,23 +7,23 @@ import { traceVerbose } from '../../common/logger';
import { areSameEnvironment, PythonEnvInfo } from '../base/info';
import { InterpreterInformation } from '../base/info/interpreter';
import {
ILocator, IPythonEnvsIterator, PythonEnvUpdatedEvent, QueryForEvent,
ILocator, IPythonEnvsIterator, PythonEnvUpdatedEvent, PythonLocatorQuery,
} from '../base/locator';
import { PythonEnvsChangedEvent } from '../base/watcher';
import { IEnvironmentInfoService } from '../info/environmentInfoService';

export class PythonEnvsResolver implements ILocator {
public get onChanged(): Event<PythonEnvsChangedEvent> {
return this.pythonEnvsReducer.onChanged;
return this.parentLocator.onChanged;
}

constructor(
private readonly pythonEnvsReducer: ILocator,
private readonly parentLocator: ILocator,
private readonly environmentInfoService: IEnvironmentInfoService,
) {}

public async resolveEnv(env: string | PythonEnvInfo): Promise<PythonEnvInfo | undefined> {
const environment = await this.pythonEnvsReducer.resolveEnv(env);
const environment = await this.parentLocator.resolveEnv(env);
if (!environment) {
return undefined;
}
Expand All @@ -34,9 +34,9 @@ export class PythonEnvsResolver implements ILocator {
return getResolvedEnv(interpreterInfo, environment);
}

public iterEnvs(query?: QueryForEvent<PythonEnvsChangedEvent>): IPythonEnvsIterator {
public iterEnvs(query?: PythonLocatorQuery): IPythonEnvsIterator {
const didUpdate = new EventEmitter<PythonEnvUpdatedEvent | null>();
const incomingIterator = this.pythonEnvsReducer.iterEnvs(query);
const incomingIterator = this.parentLocator.iterEnvs(query);
const iterator: IPythonEnvsIterator = this.iterEnvsIterator(incomingIterator, didUpdate);
iterator.onUpdated = didUpdate.event;
return iterator;
Expand Down
58 changes: 29 additions & 29 deletions src/test/pythonEnvironments/collection/environmentsResolver.unit.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,10 @@ suite('Environments Resolver', () => {
const env3 = createEnv('env3', '2.7', PythonEnvKind.System, path.join('path', 'to', 'exec3'));
const env4 = createEnv('env4', '3.9.0rc2', PythonEnvKind.Unknown, path.join('path', 'to', 'exec2'));
const environmentsToBeIterated = [env1, env2, env3, env4];
const pythonEnvReducer = new SimpleLocator(environmentsToBeIterated);
const reducer = new PythonEnvsResolver(pythonEnvReducer, new EnvironmentInfoService());
const parentLocator = new SimpleLocator(environmentsToBeIterated);
const resolver = new PythonEnvsResolver(parentLocator, new EnvironmentInfoService());

const iterator = reducer.iterEnvs();
const iterator = resolver.iterEnvs();
const envs = await getEnvs(iterator);

assert.deepEqual(envs, environmentsToBeIterated);
Expand All @@ -72,11 +72,11 @@ suite('Environments Resolver', () => {
const env1 = createEnv('env1', '3.5.12b1', PythonEnvKind.Unknown, path.join('path', 'to', 'exec1'));
const env2 = createEnv('env2', '3.8.1', PythonEnvKind.Unknown, path.join('path', 'to', 'exec2'));
const environmentsToBeIterated = [env1, env2];
const pythonEnvReducer = new SimpleLocator(environmentsToBeIterated);
const parentLocator = new SimpleLocator(environmentsToBeIterated);
const onUpdatedEvents: (PythonEnvUpdatedEvent | null)[] = [];
const reducer = new PythonEnvsResolver(pythonEnvReducer, new EnvironmentInfoService());
const resolver = new PythonEnvsResolver(parentLocator, new EnvironmentInfoService());

const iterator = reducer.iterEnvs(); // Act
const iterator = resolver.iterEnvs(); // Act

// Assert
let { onUpdated } = iterator;
Expand Down Expand Up @@ -107,11 +107,11 @@ suite('Environments Resolver', () => {
const updatedEnv = createEnv('env1', '3.8.1', PythonEnvKind.System, path.join('path', 'to', 'exec'));
const environmentsToBeIterated = [env];
const didUpdate = new EventEmitter<PythonEnvUpdatedEvent | null>();
const pythonEnvReducer = new SimpleLocator(environmentsToBeIterated, { onUpdated: didUpdate.event });
const parentLocator = new SimpleLocator(environmentsToBeIterated, { onUpdated: didUpdate.event });
const onUpdatedEvents: (PythonEnvUpdatedEvent | null)[] = [];
const reducer = new PythonEnvsResolver(pythonEnvReducer, new EnvironmentInfoService());
const resolver = new PythonEnvsResolver(parentLocator, new EnvironmentInfoService());

const iterator = reducer.iterEnvs(); // Act
const iterator = resolver.iterEnvs(); // Act

// Assert
let { onUpdated } = iterator;
Expand Down Expand Up @@ -143,18 +143,18 @@ suite('Environments Resolver', () => {
});
});

test('onChanged fires iff onChanged from reducer fires', () => {
const pythonEnvReducer = new SimpleLocator([]);
test('onChanged fires iff onChanged from resolver fires', () => {
const parentLocator = new SimpleLocator([]);
const event1: PythonEnvsChangedEvent = {};
const event2: PythonEnvsChangedEvent = { kind: PythonEnvKind.Unknown };
const expected = [event1, event2];
const reducer = new PythonEnvsResolver(pythonEnvReducer, new EnvironmentInfoService());
const resolver = new PythonEnvsResolver(parentLocator, new EnvironmentInfoService());

const events: PythonEnvsChangedEvent[] = [];
reducer.onChanged((e) => events.push(e));
resolver.onChanged((e) => events.push(e));

pythonEnvReducer.fire(event1);
pythonEnvReducer.fire(event2);
parentLocator.fire(event1);
parentLocator.fire(event2);

assert.deepEqual(events, expected);
});
Expand All @@ -178,30 +178,30 @@ suite('Environments Resolver', () => {
stubShellExec.restore();
});

test('Calls into reducer to get resolved environment, then calls environnment service to resolve environment further and return it', async () => {
test('Calls into parent locator to get resolved environment, then calls environnment service to resolve environment further and return it', async () => {
const env = createEnv('env1', '3.8', PythonEnvKind.Unknown, path.join('path', 'to', 'exec'));
const resolvedEnvReturnedByReducer = createEnv(
'env1',
'3.8.1',
PythonEnvKind.Conda,
'resolved/path/to/exec',
);
const pythonEnvReducer = new SimpleLocator([], {
const parentLocator = new SimpleLocator([], {
resolve: async (e: PythonEnvInfo) => {
if (e === env) {
return resolvedEnvReturnedByReducer;
}
throw new Error('Incorrect environment sent to the reducer');
throw new Error('Incorrect environment sent to the resolver');
},
});
const reducer = new PythonEnvsResolver(pythonEnvReducer, new EnvironmentInfoService());
const resolver = new PythonEnvsResolver(parentLocator, new EnvironmentInfoService());

const expected = await reducer.resolveEnv(env);
const expected = await resolver.resolveEnv(env);

assert.deepEqual(expected, createExpectedEnvInfo(resolvedEnvReturnedByReducer));
});

test('If the reducer resolves environment, but fetching interpreter info returns undefined, return undefined', async () => {
test('If the parent locator resolves environment, but fetching interpreter info returns undefined, return undefined', async () => {
stubShellExec.returns(
new Promise<ExecutionResult<string>>((_resolve, reject) => {
reject();
Expand All @@ -214,29 +214,29 @@ suite('Environments Resolver', () => {
PythonEnvKind.Conda,
'resolved/path/to/exec',
);
const pythonEnvReducer = new SimpleLocator([], {
const parentLocator = new SimpleLocator([], {
resolve: async (e: PythonEnvInfo) => {
if (e === env) {
return resolvedEnvReturnedByReducer;
}
throw new Error('Incorrect environment sent to the reducer');
throw new Error('Incorrect environment sent to the resolver');
},
});
const reducer = new PythonEnvsResolver(pythonEnvReducer, new EnvironmentInfoService());
const resolver = new PythonEnvsResolver(parentLocator, new EnvironmentInfoService());

const expected = await reducer.resolveEnv(env);
const expected = await resolver.resolveEnv(env);

assert.deepEqual(expected, undefined);
});

test("If the reducer isn't able to resolve environment, return undefined", async () => {
test("If the parent locator isn't able to resolve environment, return undefined", async () => {
const env = createEnv('env', '3.8', PythonEnvKind.Unknown, path.join('path', 'to', 'exec'));
const pythonEnvReducer = new SimpleLocator([], {
const parentLocator = new SimpleLocator([], {
resolve: async () => undefined,
});
const reducer = new PythonEnvsResolver(pythonEnvReducer, new EnvironmentInfoService());
const resolver = new PythonEnvsResolver(parentLocator, new EnvironmentInfoService());

const expected = await reducer.resolveEnv(env);
const expected = await resolver.resolveEnv(env);

assert.deepEqual(expected, undefined);
});
Expand Down

0 comments on commit 0963a07

Please sign in to comment.