Skip to content

Commit 04ad2c0

Browse files
jakebaileybschnurr
jakebailey
and
bschnurr
committed
Add new tryPylance banner experiments (#14759)
Co-authored-by: Bill Schnurr <[email protected]>
1 parent dec1039 commit 04ad2c0

File tree

4 files changed

+136
-48
lines changed

4 files changed

+136
-48
lines changed

‎src/client/activation/jedi/multiplexingActivator.ts‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
// Copyright (c) Microsoft Corporation. All rights reserved.
22
// Licensed under the MIT License.
3-
import { inject, injectable } from 'inversify';
3+
import { inject, injectable, named } from 'inversify';
44
import {
55
CancellationToken,
66
CompletionContext,
@@ -13,9 +13,16 @@ import {
1313
} from 'vscode';
1414
// tslint:disable-next-line: import-name
1515
import { IWorkspaceService } from '../../common/application/types';
16+
import { isTestExecution } from '../../common/constants';
1617
import { JediLSP } from '../../common/experiments/groups';
1718
import { IFileSystem } from '../../common/platform/types';
18-
import { IConfigurationService, IExperimentService, Resource } from '../../common/types';
19+
import {
20+
BANNER_NAME_PROPOSE_LS,
21+
IConfigurationService,
22+
IExperimentService,
23+
IPythonExtensionBanner,
24+
Resource
25+
} from '../../common/types';
1926
import { IServiceManager } from '../../ioc/types';
2027
import { PythonEnvironment } from '../../pythonEnvironments/info';
2128
import { JediExtensionActivator } from '../jedi';
@@ -38,7 +45,10 @@ export class MultiplexingJediLanguageServerActivator implements ILanguageServerA
3845

3946
constructor(
4047
@inject(IServiceManager) private readonly manager: IServiceManager,
41-
@inject(IExperimentService) experimentService: IExperimentService
48+
@inject(IExperimentService) experimentService: IExperimentService,
49+
@inject(IPythonExtensionBanner)
50+
@named(BANNER_NAME_PROPOSE_LS)
51+
private proposePylancePopup: IPythonExtensionBanner
4252
) {
4353
// Check experiment service to see if using new Jedi LSP protocol
4454
this.realLanguageServerPromise = experimentService.inExperiment(JediLSP.experiment).then((inExperiment) => {
@@ -56,6 +66,9 @@ export class MultiplexingJediLanguageServerActivator implements ILanguageServerA
5666
}
5767
public async start(resource: Resource, interpreter: PythonEnvironment | undefined): Promise<void> {
5868
const realServer = await this.realLanguageServerPromise;
69+
if (!isTestExecution()) {
70+
this.proposePylancePopup.showBanner().ignoreErrors();
71+
}
5972
return realServer.start(resource, interpreter);
6073
}
6174
public activate(): void {

‎src/client/common/experiments/groups.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,9 @@ export enum DeprecatePythonPath {
5555

5656
// Experiment to offer switch to Pylance language server
5757
export enum TryPylance {
58-
experiment = 'tryPylance'
58+
experiment = 'tryPylance',
59+
jediPrompt1 = 'tryPylancePromptText1',
60+
jediPrompt2 = 'tryPylancePromptText2'
5961
}
6062

6163
// Experiment for the content of the tip being displayed on first extension launch:

‎src/client/languageServices/proposeLanguageServerBanner.ts‎

Lines changed: 30 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -50,10 +50,6 @@ export class ProposePylanceBanner implements IPythonExtensionBanner {
5050
) {}
5151

5252
public get enabled(): boolean {
53-
const lsType = this.configuration.getSettings().languageServer ?? LanguageServerType.Jedi;
54-
if (lsType === LanguageServerType.Jedi || lsType === LanguageServerType.Node) {
55-
return false;
56-
}
5753
return this.persistentState.createGlobalPersistentState<boolean>(ProposeLSStateKeys.ShowBanner, true).value;
5854
}
5955

@@ -62,13 +58,13 @@ export class ProposePylanceBanner implements IPythonExtensionBanner {
6258
return;
6359
}
6460

65-
const show = await this.shouldShowBanner();
66-
if (!show) {
61+
const message = await this.getPromptMessage();
62+
if (!message) {
6763
return;
6864
}
6965

7066
const response = await this.appShell.showInformationMessage(
71-
Pylance.proposePylanceMessage(),
67+
message,
7268
Pylance.tryItNow(),
7369
Common.bannerLabelNo(),
7470
Pylance.remindMeLater()
@@ -89,19 +85,36 @@ export class ProposePylanceBanner implements IPythonExtensionBanner {
8985
sendTelemetryEvent(EventName.LANGUAGE_SERVER_TRY_PYLANCE, undefined, { userAction });
9086
}
9187

92-
public async shouldShowBanner(): Promise<boolean> {
93-
// Do not prompt if Pylance is already installed.
94-
if (this.extensions.getExtension(PYLANCE_EXTENSION_ID)) {
95-
return false;
96-
}
97-
// Only prompt for users in experiment.
98-
const inExperiment = await this.experiments.inExperiment(TryPylance.experiment);
99-
return inExperiment && this.enabled && !this.disabledInCurrentSession;
100-
}
101-
10288
public async disable(): Promise<void> {
10389
await this.persistentState
10490
.createGlobalPersistentState<boolean>(ProposeLSStateKeys.ShowBanner, false)
10591
.updateValue(false);
10692
}
93+
94+
public async getPromptMessage(): Promise<string | undefined> {
95+
if (this.disabledInCurrentSession) {
96+
return undefined;
97+
}
98+
99+
// Do not prompt if Pylance is already installed.
100+
if (this.extensions.getExtension(PYLANCE_EXTENSION_ID)) {
101+
return undefined;
102+
}
103+
104+
const lsType = this.configuration.getSettings().languageServer ?? LanguageServerType.Jedi;
105+
106+
if (lsType === LanguageServerType.Jedi) {
107+
if (await this.experiments.inExperiment(TryPylance.jediPrompt1)) {
108+
return this.experiments.getExperimentValue<string>(TryPylance.jediPrompt1);
109+
} else if (await this.experiments.inExperiment(TryPylance.jediPrompt2)) {
110+
return this.experiments.getExperimentValue<string>(TryPylance.jediPrompt2);
111+
}
112+
} else if (lsType === LanguageServerType.Microsoft || lsType === LanguageServerType.None) {
113+
if (await this.experiments.inExperiment(TryPylance.experiment)) {
114+
return Pylance.proposePylanceMessage();
115+
}
116+
}
117+
118+
return undefined;
119+
}
107120
}

‎src/test/testing/banners/proposeNewLanguageServerBanner.unit.test.ts‎

Lines changed: 87 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -28,21 +28,38 @@ import * as Telemetry from '../../../client/telemetry';
2828
import { EventName } from '../../../client/telemetry/constants';
2929

3030
interface IExperimentLsCombination {
31-
inExperiment: boolean;
31+
experiment?: TryPylance;
3232
lsType: LanguageServerType;
3333
shouldShowBanner: boolean;
3434
}
3535
const testData: IExperimentLsCombination[] = [
36-
{ inExperiment: true, lsType: LanguageServerType.None, shouldShowBanner: true },
37-
{ inExperiment: true, lsType: LanguageServerType.Microsoft, shouldShowBanner: true },
38-
{ inExperiment: true, lsType: LanguageServerType.Node, shouldShowBanner: false },
39-
{ inExperiment: true, lsType: LanguageServerType.Jedi, shouldShowBanner: false },
40-
{ inExperiment: false, lsType: LanguageServerType.None, shouldShowBanner: false },
41-
{ inExperiment: false, lsType: LanguageServerType.Microsoft, shouldShowBanner: false },
42-
{ inExperiment: false, lsType: LanguageServerType.Node, shouldShowBanner: false },
43-
{ inExperiment: false, lsType: LanguageServerType.Jedi, shouldShowBanner: false }
36+
{ experiment: undefined, lsType: LanguageServerType.None, shouldShowBanner: false },
37+
{ experiment: undefined, lsType: LanguageServerType.Microsoft, shouldShowBanner: false },
38+
{ experiment: undefined, lsType: LanguageServerType.Node, shouldShowBanner: false },
39+
{ experiment: undefined, lsType: LanguageServerType.Jedi, shouldShowBanner: false },
40+
41+
{ experiment: TryPylance.experiment, lsType: LanguageServerType.None, shouldShowBanner: true },
42+
{ experiment: TryPylance.experiment, lsType: LanguageServerType.Microsoft, shouldShowBanner: true },
43+
{ experiment: TryPylance.experiment, lsType: LanguageServerType.Node, shouldShowBanner: false },
44+
{ experiment: TryPylance.experiment, lsType: LanguageServerType.Jedi, shouldShowBanner: false },
45+
46+
{ experiment: TryPylance.jediPrompt1, lsType: LanguageServerType.None, shouldShowBanner: false },
47+
{ experiment: TryPylance.jediPrompt1, lsType: LanguageServerType.Microsoft, shouldShowBanner: false },
48+
{ experiment: TryPylance.jediPrompt1, lsType: LanguageServerType.Node, shouldShowBanner: false },
49+
{ experiment: TryPylance.jediPrompt1, lsType: LanguageServerType.Jedi, shouldShowBanner: true },
50+
51+
{ experiment: TryPylance.jediPrompt2, lsType: LanguageServerType.None, shouldShowBanner: false },
52+
{ experiment: TryPylance.jediPrompt2, lsType: LanguageServerType.Microsoft, shouldShowBanner: false },
53+
{ experiment: TryPylance.jediPrompt2, lsType: LanguageServerType.Node, shouldShowBanner: false },
54+
{ experiment: TryPylance.jediPrompt2, lsType: LanguageServerType.Jedi, shouldShowBanner: true }
4455
];
4556

57+
const expectedMessages = {
58+
[TryPylance.experiment]: Pylance.proposePylanceMessage(),
59+
[TryPylance.jediPrompt1]: 'Message for jediPrompt1',
60+
[TryPylance.jediPrompt2]: 'Message for jediPrompt2'
61+
};
62+
4663
suite('Propose Pylance Banner', () => {
4764
let config: typemoq.IMock<IConfigurationService>;
4865
let appShell: typemoq.IMock<IApplicationShell>;
@@ -51,7 +68,6 @@ suite('Propose Pylance Banner', () => {
5168
let sendTelemetryStub: sinon.SinonStub;
5269
let telemetryEvent: { eventName: EventName; properties: { userAction: string } } | undefined;
5370

54-
const message = Pylance.proposePylanceMessage();
5571
const yes = Pylance.tryItNow();
5672
const no = Common.bannerLabelNo();
5773
const later = Pylance.remindMeLater();
@@ -81,43 +97,59 @@ suite('Propose Pylance Banner', () => {
8197
});
8298

8399
testData.forEach((t) => {
84-
test(`${t.inExperiment ? 'In' : 'Not in'} experiment and "python.languageServer": "${t.lsType}" should ${
100+
test(`${t.experiment} experiment and "python.languageServer": "${t.lsType}" should ${
85101
t.shouldShowBanner ? 'show' : 'not show'
86102
} banner`, async () => {
87103
settings.setup((x) => x.languageServer).returns(() => t.lsType);
88-
const testBanner = preparePopup(true, appShell.object, appEnv.object, config.object, t.inExperiment, false);
89-
const actual = await testBanner.shouldShowBanner();
90-
expect(actual).to.be.equal(t.shouldShowBanner, `shouldShowBanner() returned ${actual}`);
104+
const testBanner = preparePopup(true, appShell.object, appEnv.object, config.object, t.experiment, false);
105+
const message = await testBanner.getPromptMessage();
106+
if (t.experiment) {
107+
expect(message).to.be.equal(
108+
t.shouldShowBanner ? expectedMessages[t.experiment] : undefined,
109+
`getPromptMessage() returned ${message}`
110+
);
111+
} else {
112+
expect(message).to.be.equal(undefined, `message should be undefined`);
113+
}
91114
});
92115
});
93116
testData.forEach((t) => {
94117
test(`When Pylance is installed, banner should not be shown when "python.languageServer": "${t.lsType}"`, async () => {
95118
settings.setup((x) => x.languageServer).returns(() => t.lsType);
96-
const testBanner = preparePopup(true, appShell.object, appEnv.object, config.object, t.inExperiment, true);
97-
const actual = await testBanner.shouldShowBanner();
98-
expect(actual).to.be.equal(false, `shouldShowBanner() returned ${actual}`);
119+
const testBanner = preparePopup(true, appShell.object, appEnv.object, config.object, t.experiment, true);
120+
const message = await testBanner.getPromptMessage();
121+
expect(message).to.be.equal(undefined, `getPromptMessage() returned ${message}`);
99122
});
100123
});
101124
test('Do not show banner when it is disabled', async () => {
125+
settings.setup((x) => x.languageServer).returns(() => LanguageServerType.Microsoft);
102126
appShell
103127
.setup((a) =>
104128
a.showInformationMessage(
105-
typemoq.It.isValue(message),
129+
typemoq.It.isValue(expectedMessages[TryPylance.experiment]),
106130
typemoq.It.isValue(yes),
107131
typemoq.It.isValue(no),
108132
typemoq.It.isValue(later)
109133
)
110134
)
111135
.verifiable(typemoq.Times.never());
112-
const testBanner = preparePopup(false, appShell.object, appEnv.object, config.object, true, false);
136+
const testBanner = preparePopup(
137+
false,
138+
appShell.object,
139+
appEnv.object,
140+
config.object,
141+
TryPylance.experiment,
142+
false
143+
);
113144
await testBanner.showBanner();
114145
appShell.verifyAll();
115146
});
116147
test('Clicking No should disable the banner', async () => {
148+
settings.setup((x) => x.languageServer).returns(() => LanguageServerType.Microsoft);
117149
appShell
118150
.setup((a) =>
119151
a.showInformationMessage(
120-
typemoq.It.isValue(message),
152+
typemoq.It.isValue(expectedMessages[TryPylance.experiment]),
121153
typemoq.It.isValue(yes),
122154
typemoq.It.isValue(no),
123155
typemoq.It.isValue(later)
@@ -127,7 +159,14 @@ suite('Propose Pylance Banner', () => {
127159
.verifiable(typemoq.Times.once());
128160
appShell.setup((a) => a.openUrl(getPylanceExtensionUri(appEnv.object))).verifiable(typemoq.Times.never());
129161

130-
const testBanner = preparePopup(true, appShell.object, appEnv.object, config.object, true, false);
162+
const testBanner = preparePopup(
163+
true,
164+
appShell.object,
165+
appEnv.object,
166+
config.object,
167+
TryPylance.experiment,
168+
false
169+
);
131170
await testBanner.showBanner();
132171

133172
expect(testBanner.enabled).to.be.equal(false, 'Banner should be permanently disabled when user clicked No');
@@ -140,10 +179,11 @@ suite('Propose Pylance Banner', () => {
140179
});
141180
});
142181
test('Clicking Later should disable banner in session', async () => {
182+
settings.setup((x) => x.languageServer).returns(() => LanguageServerType.Microsoft);
143183
appShell
144184
.setup((a) =>
145185
a.showInformationMessage(
146-
typemoq.It.isValue(message),
186+
typemoq.It.isValue(expectedMessages[TryPylance.experiment]),
147187
typemoq.It.isValue(yes),
148188
typemoq.It.isValue(no),
149189
typemoq.It.isValue(later)
@@ -153,7 +193,14 @@ suite('Propose Pylance Banner', () => {
153193
.verifiable(typemoq.Times.once());
154194
appShell.setup((a) => a.openUrl(getPylanceExtensionUri(appEnv.object))).verifiable(typemoq.Times.never());
155195

156-
const testBanner = preparePopup(true, appShell.object, appEnv.object, config.object, true, false);
196+
const testBanner = preparePopup(
197+
true,
198+
appShell.object,
199+
appEnv.object,
200+
config.object,
201+
TryPylance.experiment,
202+
false
203+
);
157204
await testBanner.showBanner();
158205

159206
expect(testBanner.enabled).to.be.equal(
@@ -171,10 +218,11 @@ suite('Propose Pylance Banner', () => {
171218
});
172219
});
173220
test('Clicking Yes opens the extension marketplace entry', async () => {
221+
settings.setup((x) => x.languageServer).returns(() => LanguageServerType.Microsoft);
174222
appShell
175223
.setup((a) =>
176224
a.showInformationMessage(
177-
typemoq.It.isValue(message),
225+
typemoq.It.isValue(expectedMessages[TryPylance.experiment]),
178226
typemoq.It.isValue(yes),
179227
typemoq.It.isValue(no),
180228
typemoq.It.isValue(later)
@@ -184,7 +232,14 @@ suite('Propose Pylance Banner', () => {
184232
.verifiable(typemoq.Times.once());
185233
appShell.setup((a) => a.openUrl(getPylanceExtensionUri(appEnv.object))).verifiable(typemoq.Times.once());
186234

187-
const testBanner = preparePopup(true, appShell.object, appEnv.object, config.object, true, false);
235+
const testBanner = preparePopup(
236+
true,
237+
appShell.object,
238+
appEnv.object,
239+
config.object,
240+
TryPylance.experiment,
241+
false
242+
);
188243
await testBanner.showBanner();
189244

190245
expect(testBanner.enabled).to.be.equal(false, 'Banner should be permanently disabled after opening store URL');
@@ -205,7 +260,7 @@ function preparePopup(
205260
appShell: IApplicationShell,
206261
appEnv: IApplicationEnvironment,
207262
config: IConfigurationService,
208-
inExperiment: boolean,
263+
experiment: TryPylance | undefined,
209264
pylanceInstalled: boolean
210265
): ProposePylanceBanner {
211266
const myfactory = typemoq.Mock.ofType<IPersistentStateFactory>();
@@ -237,7 +292,12 @@ function preparePopup(
237292
});
238293

239294
const experiments = typemoq.Mock.ofType<IExperimentService>();
240-
experiments.setup((x) => x.inExperiment(TryPylance.experiment)).returns(() => Promise.resolve(inExperiment));
295+
Object.values(TryPylance).forEach((exp) => {
296+
experiments.setup((x) => x.inExperiment(exp)).returns(() => Promise.resolve(exp === experiment));
297+
if (exp !== TryPylance.experiment) {
298+
experiments.setup((x) => x.getExperimentValue(exp)).returns(() => Promise.resolve(expectedMessages[exp]));
299+
}
300+
});
241301

242302
const extensions = typemoq.Mock.ofType<IExtensions>();
243303
// tslint:disable-next-line: no-any

0 commit comments

Comments
 (0)