-
Notifications
You must be signed in to change notification settings - Fork 62
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
feat(chat): automatically pick the database & collection if there exists only one VSCODE-610 #863
Conversation
…de into gagik/add-test-filtering
…de into gagik/one-database-handling
…-collection-handling
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.
Couple code quality comments, changes look good though.
Co-authored-by: Rhys <Anemy@users.noreply.github.com>
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.
Looks reasonable - I have mostly code style comments/questions.
src/participant/participant.ts
Outdated
// If the database name could not get automatically selected, | ||
// then the user has been prompted for it instead. | ||
if (!databaseName) { | ||
return { databaseName, collectionName }; | ||
} |
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'm not sure I understand how this works. As far as I can follow this, if I don't have any databases, I'd get the "No databases found" error and no prompt to create/select a new one.
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.
The comment was not describing everything so I updated that. databaseName ends up as undefined either because we don't have it set yet (so the user has been prompted for it) or if there's an error. It's handled by the _getOrAskFor...
function.
…-collection-handling
@@ -1026,7 +1033,7 @@ suite('Participant Controller Test Suite', function () { | |||
const chatResult = await invokeChatHandler(chatRequestMock); | |||
const askForDBMessage = chatStreamStub.markdown.getCall(0).args[0]; | |||
expect(askForDBMessage).to.include( | |||
'What is the name of the database you would like this query to run against?' | |||
'Which database would you like this query to run against? Select one by either clicking on an item in the list or typing the name manually in the chat.\n\n' |
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.
@nirinchev @Anemy I combined the message used by handleEmptyNamespaceMessage
and regular namespace prompting. It's nice to mention the option to type it manually right away and just repeat the same exact prompt if they send an empty message, that also lets us re-use the_getOrAskForMissingNamespace
helper in handleEmptyNamespaceMessage
.
…-collection-handling
…-collection-handling
VSCODE-610 mentions also behavior for cases where is 0, I will make a follow up PR as this one is getting too large. This handles the case where there is only 1 database or collection, so we'd want to pick that instead of prompting the user.
I was originally planning to split them, but it ended up being nicer organization wise to combine collection and database picking logic as a lot of it is mirrored and uses some helper methods.
Description
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes