-
Notifications
You must be signed in to change notification settings - Fork 71
feat: initial feature-flagged, draft new autocompleter MONGOSH-2036 #2424
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@@ -72,6 +72,7 @@ describe('snippet integration tests', function () { | |||
shell.assertNoErrors(); | |||
}); | |||
|
|||
// TODO: port to the new autocomplete |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll either do this in this PR or I'll file a follow-up ticket
@@ -29,6 +29,7 @@ export interface InterpreterEnvironment { | |||
*/ | |||
export class OpenContextRuntime implements Runtime { | |||
private interpreterEnvironment: InterpreterEnvironment; | |||
// TODO: we have to also port this to the new autocomplete |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I might even do it in this PR)
@@ -352,6 +363,14 @@ describe('MongoshNodeRepl', function () { | |||
}; | |||
const tabtab = async () => { | |||
await tab(); | |||
if (process.env.USE_NEW_AUTOCOMPLETE) { | |||
// TODO: This is because autocomplete() is async and will either list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Less of a TODO than a NOTE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I feel like setting a fixed timeout of 210 is either almost guaranteed flakiness or something that at least looks like it would be flaky ... can we somehow wait for the right event (e.g. some autocompletion data fteched event) at this point?
properties of the things that match. | ||
*/ | ||
//console.log(name, kind); | ||
// TODO: This can be improved further if we first see if there are any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed a ticket to just improve the filtering: https://jira.mongodb.org/browse/MONGOSH-2207
line: string, | ||
results: { result: string }[] | ||
): [string[], string] | [string[], string, 'exclusive'] { | ||
// TODO: actually use 'exclusive' when we should |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -352,6 +363,14 @@ describe('MongoshNodeRepl', function () { | |||
}; | |||
const tabtab = async () => { | |||
await tab(); | |||
if (process.env.USE_NEW_AUTOCOMPLETE) { | |||
// TODO: This is because autocomplete() is async and will either list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I feel like setting a fixed timeout of 210 is either almost guaranteed flakiness or something that at least looks like it would be flaky ... can we somehow wait for the right event (e.g. some autocompletion data fteched event) at this point?
const hash = crypto.createHash('sha256'); | ||
hash.update(uri); | ||
return hash.digest('hex'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like it miiiight be worth caching on the Mongo
instance, not sure if that would be premature optimization but while hashing is cheap, it might not be do-a-few-times-per-autocomplete cheap?
return { | ||
currentDatabaseAndConnection: () => { | ||
return { | ||
connectionId: connectionIdFromURI(this.currentDb.getMongo().getURI()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intended to be the ID that the autocompleter passes to things like connectionId
? Would it be simpler to just assign an increasing counter id to each Mongo
instance?
Still a draft, don't merge :)
Most of everything is stubbed or TODO and this needs tests. But hopefully it is reasonably close to the interface we're going for.
NOTE: It requires mongodb-js/devtools-shared#520, but that in turn depends on a different mongosh PR so this is broken right now.
then
then