-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[vscode] Stub Chat and Language Model API #13778
Conversation
fixes eclipse-theia#13756 contributed on behalf of STMicroelectronics Signed-off-by: Remi Schnekenburger <rschnekenburger@eclipsesource.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.
A couple of comments.
createChatParticipant(id: string, handler: theia.ChatRequestHandler): theia.ChatParticipant { | ||
return { | ||
id, | ||
requestHandler: (request: theia.ChatRequest, context: theia.ChatContext, response: theia.ChatResponseStream, token: CancellationToken) => { }, |
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 not just assign the passed-in request handler instead of a dummy?
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.
Done in new commit
readonly command?: string; | ||
|
||
private constructor(public readonly response: ReadonlyArray<theia.ChatResponseMarkdownPart | theia.ChatResponseFileTreePart | theia.ChatResponseAnchorPart | ||
| theia.ChatResponseCommandButtonPart>, public readonly result: theia.ChatResult, public readonly participant: string) { } |
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.
public
is not necessary, IMO. Goes for all similar occurrences.
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 should be addressed in the new commit.
export class LanguageModelError extends Error { | ||
|
||
static NoPermissions(message?: string): LanguageModelError { | ||
return new LanguageModelError(message, LanguageModelError.NotFound.name); |
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 seems strange when reading (https://code.visualstudio.com/api/references/vscode-api#LanguageModelError). What should the Error.name
and code
properties be when we construct a LanguageModelError
with the NoPermissions
static method?Right now, it would be
code === NotFoundand
name === LanguageModelError
, right? What am I missing?
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 fixed this part. The LanguageModelError NoPermissions/NotFound/Blocked should now have the correct code.
BTW, due to Typescript version used, the 'cause' property used in VS code in the constructor is not available yet (es2022). Is there a way to track these issues, as we do for the monaco uplift annotations? should we introduce a typescript-uplift annotation?
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 doubt this is an issue of the typescript version: we are using 5.4 at the moment, which is fairly new. I figure the problem is with the compilation target, right?
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.
yes, the problem is the target
What it does
Stub the API for chat and Language Models introduced in VS Code 1.90.
Closes #13756
Status of the report when comparing with vscode 1.90: status.zip
How to test
Follow-ups
Real implementation of the API is tracked here: #13777
Review checklist
Reminder for reviewers