-
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: improve the format of the participant answer VSCODE-582 #804
feat: improve the format of the participant answer VSCODE-582 #804
Conversation
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.
lgtm! I like that we're moving a bit more functional and keeping less things like _codeToEvaluate
around on state. I find it makes things less coupled and easier to test/abstract.
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.
A few super minor comments from me - mostly on the code, as I'm not familiar enough with the project to do a deeper review yet.
|
||
await this._openInResultPane(evaluateResponse.result); | ||
|
||
return Promise.resolve(true); |
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.
[nit] I don't believe we need the Promise.resolve
here since we're already in an async context.
return Promise.resolve(true); | |
return true; |
if ( | ||
!selections || | ||
!Array.isArray(selections) || | ||
(selections.length === 1 && this._getSelectedText(selections[0]) === '') | ||
) { |
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 know this is code that didn't change in this PR, but it's counterintuitive to me that we need to do these evaluations here, considering we already have this._selectedText
. I would expect that this condition is equivalent to !this._selectedText
.
} | ||
|
||
return this._evaluatePlayground(); | ||
return this._evaluatePlayground(codeToEvaluate); |
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.
[nit] With the current code structure, we may be passing undefined
here, while _evaluatePlayground
expects string. My suggestion would be to declare the type of codeToEvaluate
and either return early in an else
clause for the if
-statement on line 690, or fallback to an empty string.
Ugh, didn't notice the PR was merged prior to posting my comments - they're not blocking and can be addressed as a drive-by in the future. |
Description
Checklist
Motivation and Context
Types of changes