-
Notifications
You must be signed in to change notification settings - Fork 79
feat(autocomplete): add show/use completions MONGOSH-563 #829
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -137,11 +137,19 @@ class MongoshNodeRepl implements EvaluationListener { | |
this.insideAutoComplete = true; | ||
try { | ||
// Merge the results from the repl completer and the mongosh completer. | ||
const [ [replResults], [mongoshResults] ] = await Promise.all([ | ||
const [ [replResults], [mongoshResults,, mongoshResultsExclusive] ] = await Promise.all([ | ||
(async() => await origReplCompleter(text) || [[]])(), | ||
(async() => await mongoshCompleter(text))() | ||
]); | ||
this.bus.emit('mongosh:autocompletion-complete'); // For testing. | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I think I've got a bit better the context. Makes sense. |
||
// Sometimes the mongosh completion knows that what it is doing is right, | ||
// and that autocompletion based on inspecting the actual objects that | ||
// are being accessed will not be helpful, e.g. in `use a<tab>`, we know | ||
// that we want *only* database names and not e.g. `assert`. | ||
if (mongoshResultsExclusive) { | ||
return [mongoshResults, text]; | ||
} | ||
// Remove duplicates, because shell API methods might otherwise show | ||
// up in both completions. | ||
const deduped = [...new Set([...replResults, ...mongoshResults])]; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -84,7 +84,9 @@ describe('Database', () => { | |
returnType: 'AggregationCursor', | ||
platforms: ALL_PLATFORMS, | ||
topologies: ALL_TOPOLOGIES, | ||
serverVersions: ALL_SERVER_VERSIONS | ||
serverVersions: ALL_SERVER_VERSIONS, | ||
isDirectShellCommand: false, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a custom "autocompleter" for arguments? And this will work with a worker thread as well cause the function will be called inside the runtime right? Slowly getting it :). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The entry below is, yes.
I think so, yes. The autocompleter in the browser also lives inside the same environment as e.g. the |
||
shellCommandCompleter: undefined | ||
}); | ||
}); | ||
it('hasAsyncChild', () => { | ||
|
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.
Why is this necessary? Will this also work in the browser?
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.
@mcasimir Are you referring to the
'exclusive'
flag here?The mongosh-repl.ts changes have a somewhat more specific context here, but the gist of it is that we run both the Node.js default autocompleter and our own there and combine the results; but the Node.js REPL thinks that it should complete e.g.
use a
touse assert
, because it just looks at thea
and finds global variables with the same name.I don’t think the extra array entry should hurt, we’re only using the first entry anyway:
mongosh/packages/browser-runtime-core/src/autocompleter/shell-api-autocompleter.ts
Line 22 in 831220a