-
Notifications
You must be signed in to change notification settings - Fork 304
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(autoedit): refactor renderer code to simplify iteration on decor… #6163
Conversation
} | ||
} | ||
|
||
export interface AutoeditsDecorator extends vscode.Disposable { |
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.
Please add documentation describing what the interface contract is. Interfaces generally should be documented thoroughly to indicate behavior/constraints/invariants that implementors and callers should satisfy.
I see two places where this interface is used, (1) one in the main auto-edits command and (2) the other in the test command. In (1), clearDecorations
and dispose
are called together. In (2), it doesn't appear that dispose
is ever called. So some clarity around how concrete implementations should be used would clarifying here. Should instances be kept around and used for multiple rounds of displaying decorations, or should new instances be created whenever new decorations are shown.
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.
Nice Insight !!
clearDecorations
and dispose
were coupled and dispose
should anyways clear the decorations.
Added in the documentation that each decorator instance should be used only for one decoration and disposed after the decoration is used, and we should clear the decoration on dispose.
Does the updated implementation look okay ?
@@ -75,7 +76,9 @@ export class AutoeditsProvider implements vscode.InlineCompletionItemProvider, v | |||
contextRankingStrategy: ContextRankingStrategy.TimeBased, | |||
dataCollectionEnabled: false, | |||
}) | |||
this.rendererManager = new AutoEditsRendererManager() | |||
this.rendererManager = new AutoEditsRendererManager( | |||
DecorationStrategyIdentifier.DefaultDecorator |
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.
Here, we're passing an enum-switch around, which means that multiple places in code (here and then in AutoEditsRendererManager) have to be aware of this enum, which makes the code more brittle, hard to update, and hard to test. If more cases are added in the future, we need to remember to add the appropriate switch cases and handling in tests.
It would be cleaner to pass a function here that creates the Decorator instance we want the AutoEditsRendererManager instance to use. So something like this:
new AutoEditsRendererManager(
(editor: vscode.TextEditor) => new DefaultDecorator(editor)
)
And then in manager.ts
:
export class AutoEditsRendererManager implements vscode.Disposable {
constructor(private createDecorator: (editor: vscode.TextEditor) => AutoeditsDecorator) { ... }
public async showEdit(options: AutoEditsManagerOptions): Promise<void> {
...
this.activeEdit = {
uri: options.document.uri.toString(),
range: options.range,
prediction: options.prediction,
decorator: this.createDecorator(editor),
}
...
}
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.
That's a good insight. Modified the changes as per suggestion !!
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.
Please address the one remaining change, otherwise LGTM!
I'll merge this one to merge my follow-up PR into the main branch. It will be less risky because my PR refactors more logic for which we don't have test coverage yet. |
The PR refactors the current autoedits renderer to use a common schema for which Decorations can use.
Idea is to make it easy for anyone to try to implement a new decoration type without knowing the details on diff computation.
The common schema is in the file here
Test plan
Testing examples in
cody-chat-eval
and playing with the rendere to make sure we don't have any regression