Show the debugger survey banner for only a subset of users. by ericsnowcurrently · Pull Request #2425 · microsoft/vscode-python

ericsnowcurrently · 2018-08-21T20:36:38Z

(fixes #2300)

Title summarizes what is changing
Includes a news entry file (remember to thank yourself!)
Unit tests & code coverage are not adversely affected (within reason)
Works on all actively maintained versions of Python (e.g. Python 2.7 & the latest Python 3 release)
Works on Windows 10, macOS, and Linux (e.g. considered file system case-sensitivity)
Dependencies are pinned (e.g. "1.2.3", not "^1.2.3")
package-lock.json has been regenerated if dependencies have changed

codecov · 2018-08-21T20:55:30Z

Codecov Report

Merging #2425 into master will increase coverage by 0.08%.
The diff coverage is 93.75%.

@@            Coverage Diff             @@
##           master    #2425      +/-   ##
==========================================
+ Coverage    75.4%   75.49%   +0.08%     
==========================================
  Files         309      309              
  Lines       14331    14363      +32     
  Branches     2536     2538       +2     
==========================================
+ Hits        10807    10843      +36     
+ Misses       3515     3512       -3     
+ Partials        9        8       -1

Flag	Coverage Δ
#MacOS	`74.32% <93.75%> (+0.07%)`	⬆️
#Windows	`74.4% <93.75%> (+0.06%)`	⬆️

Impacted Files	Coverage Δ
src/client/common/types.ts	`100% <100%> (ø)`	⬆️
src/client/common/serviceRegistry.ts	`100% <100%> (ø)`	⬆️
src/client/debugger/serviceRegistry.ts	`100% <100%> (ø)`	⬆️
src/client/debugger/types.ts	`100% <100%> (ø)`	⬆️
src/client/extension.ts	`95.42% <100%> (ø)`	⬆️
src/client/common/utils.ts	`74.68% <33.33%> (-1.64%)`	⬇️
src/client/debugger/banner.ts	`91.89% <95.52%> (+11.89%)`	⬆️
src/client/linters/lintingEngine.ts	`91.15% <0%> (-0.89%)`	⬇️
src/client/debugger/mainV2.ts	`68.32% <0%> (-0.36%)`	⬇️

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 6369262...8275934. Read the comment docs.

MikhailArkhipov · 2018-08-21T21:42:10Z

+
+    // "enabled" state
+
+    public isEnabled(): boolean {


readonly get?

It isn't necessarily a cheap operation. A function reflects that.

MikhailArkhipov · 2018-08-21T21:43:01Z

    createTemporaryFile(extension: string): Promise<TemporaryFile>;
 }
+
+export const IRuntime = Symbol('IRuntime');


Looks more like utilities than runtime

MikhailArkhipov · 2018-08-21T21:44:25Z

+
+    public async show(): Promise<void> {
        const appShell = this.serviceContainer.get<IApplicationShell>(IApplicationShell);
+        const msg = 'Can you please take 2 minutes to tell us how the Debugger is working for you?';


MikhailArkhipov · 2018-08-21T21:44:34Z

        const yes = 'Yes, take survey now';
        const no = 'No thanks';
-        const response = await appShell.showInformationMessage('Can you please take 2 minutes to tell us how the Debugger is working for you?', yes, no);
+        const later = 'Remind me Later';


MikhailArkhipov · 2018-08-21T21:46:11Z

+    ) { }
+
    public initialize() {
        if (this.initialized) {


can it simply init itself in ctor?

Apparently constructors are meant to contain only trivial initialization operations. Hence a separate initialize() method.

But it has to be called anyway right after the construction? What does it save?

d3r3kk

I think your solution works. Consider my feedback on caching all the services on init, but it's definitely not necessary. @MikhailArkhipov's suggestions on wording/names are good ideas too.

d3r3kk · 2018-08-21T22:39:16Z

+    // "enabled" state
+
+    public isEnabled(): boolean {
+        const factory = this.serviceContainer.get<IPersistentStateFactory>(IPersistentStateFactory);


Any way we can cache this at the initialize step in the class? Probably not going to make a huge difference, but perhaps it will speed some things along.

I'm going to punt on this. :) There are a lot of little improvements that could be made. I had to stop myself. :)

d3r3kk · 2018-08-21T22:51:34Z

+            const runtime = this.serviceContainer.get<IRuntime>(IRuntime);
+            const randomSample = runtime.getRandomInt(0, 100);
+            selected = randomSample < SAMPLE_SIZE_PER_HUNDRED;
+            state.updateValue(selected).ignoreErrors();


You can probably just await this one, no need to use the ignoreErrors I don't think. Consider using a then-clause?

state.updateValue(selected).then(() => { do_something_on_success(); }, (failReason) => { do_something_else_on_fail(); }

This was copied from somewhere else.

ericsnowcurrently added 10 commits August 21, 2018 11:15

ExperimentalDebuggerBanner -> DebuggerBanner

1c40557

Clean up the debugger banner code a little.

38f0e25

Fix names.

3f3648d

Drop DebuggerBanner.launchSurvey().

5ca4983

Drop unused methods from interface.

80a054a

More cleanup.

279de82

Use getRandomBetween() for random ints.

028fc18

Add a "Later" option.

adf8629

Only show the banner for 10% of users.

ba739ae

Add a NEWS entry.

120eb95

ericsnowcurrently mentioned this pull request Aug 21, 2018

Show the debugger survey banner for only a subset of users. #2365

Closed

7 tasks

MikhailArkhipov reviewed Aug 21, 2018

View reviewed changes

d3r3kk approved these changes Aug 21, 2018

View reviewed changes

ericsnowcurrently added 2 commits August 21, 2018 17:24

Fix capitalization.

3572636

Runtime -> Random.

8275934

DonJayamanne approved these changes Aug 22, 2018

View reviewed changes

ericsnowcurrently merged commit 2a6043d into microsoft:master Aug 22, 2018

ericsnowcurrently deleted the fix-2300-debugger-banner-2 branch August 22, 2018 16:51

lock Bot locked as resolved and limited conversation to collaborators Jul 31, 2019

Conversation

ericsnowcurrently commented Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d3r3kk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ericsnowcurrently commented Aug 21, 2018 •

edited

Loading

codecov Bot commented Aug 21, 2018 •

edited

Loading