-
Notifications
You must be signed in to change notification settings - Fork 291
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
Hitesh/autoedits improvements #5956
base: main
Are you sure you want to change the base?
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.
Some feedback inline.
@@ -56,9 +57,10 @@ export class AutoeditsProvider implements vscode.Disposable { | |||
this.provider = new OpenAIPromptProvider() | |||
} else if (provider === 'deepseek') { | |||
this.provider = new DeepSeekPromptProvider() | |||
} else { | |||
logDebug('AutoEdits', `provider ${provider} not supported`) | |||
} else if (provider === 'fireworks') { |
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 is a good candidate for a switch
statement.
Note, the method name is misspelled: initialize
@@ -184,7 +184,7 @@ export function getCurrentFilePromptComponents( | |||
const codeToReplace = { | |||
codeToRewrite: codeToRewrite, | |||
startLine: completePrefixLines - prefixContext.codeToRewriteStartLines, | |||
endLine: completePrefixLines + suffixContext.codeToRewriteEndLines, | |||
endLine: completePrefixLines + suffixContext.codeToRewriteEndLines - 1, |
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.
Note, above you can just write codeToRewrite
instead of codeToRewrite: codeToRewrite
How is endLine
used? Can we get some unit test coverage of this?
}, | ||
] | ||
return { | ||
codeToReplace: codeToReplace, |
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.
When the property name and the variable name are the same, you can just write it once as a shorthand.
codeToReplace: codeToReplace, | |
codeToReplace, |
}) | ||
|
||
const suggesterType = vscode.window.createTextEditorDecorationType({ | ||
before: { color: GHOST_TEXT_COLOR }, | ||
after: { color: GHOST_TEXT_COLOR }, | ||
}) | ||
|
||
function combineRanges(ranges: vscode.Range[], n: number): vscode.Range[] { |
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.
Give n
a more descriptive name, and consider documenting what the function does.
if ( | ||
currentRange.end.line === nextRange.start.line && | ||
(nextRange.start.character - currentRange.end.character <= n || | ||
currentRange.intersection(nextRange)) |
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.
What are the constraints on the input ranges? Why not handle intersections in general where nextRange intersects with currentRange but doesn't happen to start on the line that the other one ends on?
) { | ||
currentRange = new vscode.Range( | ||
currentRange.start, | ||
nextRange.end.character > currentRange.end.character ? nextRange.end : currentRange.end |
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's a danger here that if someone makes a change to the currentRange.end.line === nextRange.start.line
constraint above then this result is going to be garbage. Consider something semantic, like:
currentRange.end.isAfter(nextRange.end) ? currentRange.end : nextRange.end
predictedText = predictedText.replace(/\n$/, '') | ||
|
||
if (this.activeProposedChange) { | ||
await this.dismissProposedChange() |
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 following method is pretty long, and you rely on there being no await
after this point and before you set decorations, otherwise you might render two different ones at once.
Could you introduce some helper methods and a comment to point that out and reduce the reading gap between when the old changes are dismissed and the new ones are rendered?
Context
Improving the
auto-edits
UI with the decorations. For theauto-edits
suggestions without the new lines additions, the current implementation, adds decoration. In the current editor, displaysred
color over the text we want to deleted and a green line decoration for the text which will replace the line.Scope of improvements
top: '10px'
left: '10px'
were the new lines, which we new lines that we need to add, but they overlap with the lines which we will preserve.
For now, this should be okay since it gives us a way to iterate on the model quality, while we think about the UI in parallel.
Ideas for improvements
Insert the current line in the editor itself, similar to the
inline-edits
but this introduces additional complexity to manager the undo buffers and making sure, the line additions are deleted when user do any action such as moving the cursor, switching file, ctrl+z etc. One sample implementation to add blank line is done by @olafurpg here: WIP #5963We can display a floating window to indicate the text insertion.
Test plan
Manual Examples and usability examples:
https://www.loom.com/share/c22ff1670a0a4ffb8210cae9f7e56846