Add two new popups: One to switch to new LS, another to ask for feedback. #2173
Conversation
|
@DonJayamanne & @brettcannon apologies for taking so long! Learning while doing... |
| } | ||
| )); | ||
|
|
||
| this.surveyBanner = new NewLanguageServerSurveyBanner( |
There was a problem hiding this comment.
Why aren't we resolving the survey banner instead of manually constructing it. I.e. use DI instead of manual construction.
| async (args) => { | ||
| if (this.languageClient) { | ||
| await this.startupCompleted.promise; | ||
| this.languageClient.onNotification.bind(this); |
There was a problem hiding this comment.
Thanks, I was trying to figure out how to capture the 'onNotification' event from the BaseLanguageClient worked at the time, this is just a remnant. Thanks for catching this!
| private excludedFiles: string[] = []; | ||
| private typeshedPaths: string[] = []; | ||
| private loadExtensionArgs: {} | undefined; | ||
| private surveyBanner: NewLanguageServerSurveyBanner; |
There was a problem hiding this comment.
Please use an interface, not a class.
There was a problem hiding this comment.
Can do, I think I've learned enough from doing these two popups to figure out what I can make more generic.
| import { IBrowserService, IConfigurationService, IExtensionContext, | ||
| IOutputChannel, IPersistentStateFactory, IPythonSettings } from '../common/types'; | ||
| import { IServiceContainer } from '../ioc/types'; | ||
| import { NewLanguageServerSurveyBanner } from '../languageServices/newLanguageServerSurveyBanner'; |
| function enables the popup for this user. | ||
| */ | ||
|
|
||
| export class ProposeNewLanguageServerBanner { |
There was a problem hiding this comment.
Please change to LanguageServerBanner.
We've used the prefix Experimental in the ExperimentalDebugger as that's what its called Experimental. With the language server, its the Language Server, we haven't had one before. Jedi wasn't a language server. Hope that makes sense.
There was a problem hiding this comment.
This must implement an interface. No concrete classes please.
src/client/providers/jediProxy.ts
Outdated
| this.initialized = createDeferred<void>(); | ||
| this.startLanguageServer().then(() => this.initialized.resolve()).ignoreErrors(); | ||
|
|
||
| this.proposeNewLanguageServerPopup = new ProposeNewLanguageServerBanner( |
There was a problem hiding this comment.
Please use DI container to resolve an instance of this class from an interface.
| traceLogging | ||
| }, | ||
| middleware: { | ||
| provideCompletionItem: (document: TextDocument, position: VPosition, token: CancellationToken, next: ProvideCompletionItemsSignature) => { |
There was a problem hiding this comment.
Ahhh. Where did you get the information to implement this API. GitHub or Stackoverflow?
There was a problem hiding this comment.
Github, here. microsoft/vscode#54404 (but I updated SO as well with the Q&A).
src/client/common/utils.ts
Outdated
| } | ||
| } | ||
|
|
||
| export function getRandom(): number { |
There was a problem hiding this comment.
No need to export this, its not used anywhere except in here.
There was a problem hiding this comment.
yet
Just left this as exported in case someone would just want the 0.0..1.0 values back, as a safe replacement to Math.random().
There was a problem hiding this comment.
Lets keep it simple, if not required, private scope.
Codecov Report
@@ Coverage Diff @@
## master #2173 +/- ##
==========================================
- Coverage 79.63% 78.52% -1.12%
==========================================
Files 308 310 +2
Lines 14191 14358 +167
Branches 2521 2547 +26
==========================================
- Hits 11301 11274 -27
- Misses 2878 3072 +194
Partials 12 12
Continue to review full report at Codecov.
|
- add new random function to handle 'between n and m' - still in progress, lots of WIP code yet - code for setting up new LS started, but needs further work (restart?) - launch survey for users of new LS works - NO TESTS YET! GRRRR.
- fix issue with random (pulling 8 bytes from machine id rather than 4) - update 'shouldShowBanner' a bit, remove DEREK specific flags
- update random functions to actually work properly - add some comments to the new classes to help define their purpose - remove debugging/dev code in preparation for going live
- Remove debugging code - simplify popup classes (no injection necessary) - pull out common code, put in utils (getRandom) - correct setting for config to switch to new LS
- Rename ProposeNewLanguageServerBanner to ProposeLanguageServerBanner - Remove remnant of onNotification.bind. - Rename the class with the New prefix to lose the New.
- remove export from utils.getRandom.
- Use interfaces for new popups instead of the actual class references. - Switch back to DI instead of manual construction... (hence the interface switch above).
src/client/common/types.ts
Outdated
| } | ||
|
|
||
| export const IPythonExtensionSurveyBanner = Symbol('IPythonExtensionSurveyBanner'); | ||
| export interface IPythonExtensionSurveyBanner { |
There was a problem hiding this comment.
I see you renamed IExperimenta...Banner and now we have IPythonExtensionSurveyBanner and IPythonExtensionBanner. Why aren't we using the same interface?
If we cannot, then why rename the existing interface to make it generic, when it isn't being re-used?
Anyways, lets not change this now.
In previous commit you seems to have the same API for the debugger and language server banners, and my impression was you'd be using the same Interface, but now we've attempted to create something generic, yet not.
Note, ideally we should have more than one class, one that drives the banner and the other that provides the information about the banner, then the other than acts on the data. I didn't want two-three classes for a simple banner, hence exposed crammed all in one with public methods enabled, initialize, showBanner, shouldShowBanner, disable, launchSurvey, which would ideally work in other banners.
I don't want you to change anything at this stage, lets not do that.
There was a problem hiding this comment.
Please do not change anything, lets just get this PR out.
There was a problem hiding this comment.
I agree with your assessment here 100%. I was trying to get to a point where I would have a unified API for all new banners we'll need in the extension going forward.
However, it also became apparent as I was progressing that the 'all in one' API from the original banner wasn't quite what I needed... so I allowed my work to stray somewhat from that, this is the change you are seeing through my submits.
I was learning other things while also attempting this and simply ran short on time to make things as nice as I'd otherwise like. I have a few clean-up tasks on my plate once this release goes out for sure!
...lets just get this PR out.
Agreed. It appears I have some other issues to concern myself with from what the build machines are telling me!
| this.maxShowAttempts = maxShowAttemptThreshold; | ||
| } | ||
|
|
||
| public get optionLabels(): string[] { |
There was a problem hiding this comment.
I'd rename this to options. No need to change.
There was a problem hiding this comment.
Not sure where these fit, we're exposing this as well as bannerLabels...
Anyways, the code might not yet be complete.
There was a problem hiding this comment.
Yep, sussing out what is common between the banners here. This will be a bit of a WIP until next release unfortunately.
| return this.getPythonLSLaunchCounter(); | ||
| } | ||
|
|
||
| public optionTriggerCount(label: string): number { |
There was a problem hiding this comment.
Feels like you need a better data structure, or just use enums instead of this.
This feels clunky.
There was a problem hiding this comment.
Agreed. This was me coming up with ideas for unit tests...
| let count: number = -1; | ||
| switch (label) { | ||
| case this.bannerLabels[LSSurveyLabelIndex.Yes]: { | ||
| count = this.labelTriggerCount[LSSurveyLabelIndex.Yes]; |
There was a problem hiding this comment.
In the future please use an Enum, more readable, than numbers.
Please do not change now.
There was a problem hiding this comment.
Not entirely certain what you mean here... I'm using an Enum for the indecies into my two arrays.
|
@DonJayamanne & @brettcannon: First, the (18) and (19) are the failures due to I'm going to reset everything I can, or will simply try on a freshly paved VM this morning, but I'm not feeling confident for the release at all. |
src/client/common/types.ts
Outdated
| shownCount: Promise<number>; | ||
| optionLabels: string[]; | ||
| showBanner(): Promise<void>; | ||
| optionTriggerCount(label: string): number; |
- Remove notion of 'trigger count' from popup banners altogether - my anti-pester idea was rejected for good reason - user has 'No' button to dismiss these popups forever - remove shownCount from propose LanguageServer popup - Put the 'IExperimentalDebuggerBanner' name back the way it was
- wrap it in a check for 'is test execution' - we need to block the popup during testing for some reason (TBD)
- tests for proposal popup
DonJayamanne
changed the title
2 participants
Note: Working on unit tests now. Starting the PR to get some advance feedback.
Fixes #2127
"1.2.3", not"^1.2.3")package-lock.jsonhas been regenerated if dependencies have changedPR Feedback:
TODO:
onNotification.bind.Newprefix to lose theNew.