Notebooks: Only have One Connection in Attach To Dropdown #8582
Conversation
|
These linting rules are not necessarily related to your code changes. They're setup to bring awareness to code quality issues. Some of the links may be off by a line or two. Rule: no-floating-promises
Rule: no-floating-promises
Rule: no-floating-promises
Rule: no-floating-promises
Rule: no-floating-promises
Rule: no-floating-promises
Rule: no-floating-promises
Rule: no-floating-promises
Rule: no-floating-promises
|
|
This pull request introduces 4 alerts when merging 10d5b77 into 0d9353d - view on LGTM.com new alerts:
|
|
These linting rules are not necessarily related to your code changes. They're setup to bring awareness to code quality issues. Some of the links may be off by a line or two. Rule: no-floating-promises
Rule: no-floating-promises
Rule: no-floating-promises
Rule: no-floating-promises
Rule: no-floating-promises
Rule: no-floating-promises
Rule: no-floating-promises
Rule: no-floating-promises
Rule: no-floating-promises
|
| if (showSelectConnection) { | ||
| this.loadWithSelectConnection(connections); | ||
| let connections: string[] = model.context && model.context.title ? [model.context.title] : []; | ||
| if (connections.length === 0) { |
There was a problem hiding this comment.
nit: Could just have the [] value in the line above be [msgSelectConnection] instead.
There was a problem hiding this comment.
ooh i like that. thanks.
| public static get DefaultContext(): IDefaultConnection { | ||
| let defaultConnection: ConnectionProfile = <any>{ | ||
| public static get DefaultContext(): ConnectionProfile { | ||
| return <any>{ |
There was a problem hiding this comment.
any [](start = 10, length = 3)
Why cast to any? Should this be ConnectionProfile?
There was a problem hiding this comment.
There was a problem hiding this comment.
Sigh. These are fake connection profile objects and I really really really don't like it.
There was a problem hiding this comment.
Tried to fix it but went down a bit of a rabbit hole, so may have to save that for another PR for more cleanup 😢
There was a problem hiding this comment.
Yeah. I think the approach i would try if I was fixing this would be to have a context be ConnectionProfile | DefaultContextType, that way instead of doing stuff like checking ID you just check the type of the object at runtime and use that to determine what you do.
In reply to: 354972934 [](ancestors = 354972934)
There was a problem hiding this comment.
Makes a ton of sense, thanks
| @@ -146,8 +146,8 @@ export class CodeComponent extends AngularDisposable implements OnInit, OnChange | |||
| let connectionService = this.connectionService; | |||
| if (!shouldConnect && connectionService && connectionService.isConnected(cellUri)) { | |||
| connectionService.disconnect(cellUri).catch(e => this.logService.error(e)); | |||
| } else if (shouldConnect && this._model.activeConnection && this._model.activeConnection.id !== '-1') { | |||
| connectionService.connect(this._model.activeConnection, cellUri).catch(e => this.logService.error(e)); | |||
| } else if (shouldConnect && this._model.context && this._model.context.id !== '-1') { | |||
There was a problem hiding this comment.
It would be good to have this -1 ID be defined as a const somewhere so it's clear that this is a special ID used for the default context.
| // Change the defaultConnection to newConnection | ||
| this._activeContexts.defaultConnection = newConnection; | ||
| this._activeConnection = newConnection; | ||
| } |
There was a problem hiding this comment.
The comment here isn't correct anymore. This whole method is weird anyways, I'm not sure what it's doing since the name (refreshConnections) doesn't seem to match the logic.
| conns = NotebookContexts.getActiveContexts(connService.object, [mssqlProviderName], testConn); | ||
| assert.deepStrictEqual(conns, defaultContext); | ||
| conns = NotebookContexts.getContextForKernel(testConn, [mssqlProviderName, 'fakeProvider']); | ||
| assert.deepStrictEqual(conns, testConn); |
There was a problem hiding this comment.
Would suggest adding messages to these so it's clear which one failed without having to look at line numbers
|
These linting rules are not necessarily related to your code changes. They're setup to bring awareness to code quality issues. Some of the links may be off by a line or two. Rule: no-floating-promises
Rule: no-floating-promises
Rule: no-floating-promises
Rule: no-floating-promises
Rule: no-floating-promises
Rule: no-floating-promises
Rule: no-floating-promises
Rule: no-floating-promises
Rule: no-floating-promises
|
|
This pull request fixes 1 alert when merging bb5f71f into 6fb120f - view on LGTM.com fixed alerts:
|
|
These linting rules are not necessarily related to your code changes. They're setup to bring awareness to code quality issues. Some of the links may be off by a line or two. Rule: no-floating-promises
Rule: no-floating-promises
Rule: no-floating-promises
Rule: no-floating-promises
Rule: no-floating-promises
Rule: no-floating-promises
Rule: no-floating-promises
Rule: no-floating-promises
Rule: no-floating-promises
|
|
This pull request fixes 1 alert when merging 2750e82 into de177c0 - view on LGTM.com fixed alerts:
|
* Have only one connection in attach to dropdown * LGTM fixes * Test fixes not unnecessarily changing context 2x * PR Feedback
We had overwhelming feedback that our Attach To dropdown had extremely confusing behavior, and was not predictable in what connection we would choose by default.
Changes:
I added a couple of new tests. @corivera is currently working on tests for notebookActions. I'll work with him to write some new tests for the attach to dropdown.