-
Notifications
You must be signed in to change notification settings - Fork 326
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(autoedits): make it reactive to config changes #6537
Conversation
2018bc0
to
0f22a0e
Compare
distinctUntilChanged((a, b) => { | ||
return ( | ||
isEqual(a[0].configuration, b[0].configuration) && | ||
isEqual(a[1], b[1]) && | ||
isEqual(a[2], b[2]) | ||
) | ||
}), |
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 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.
simply copied from registerAutocomplete
anything wrong with this approach 😅
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 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.
Yeah, I think there's a TODO to address it. We should look into it at some point.
return promiseFactoryToObservable(async () => { | ||
return await getAutoeditsProviderDocumentFilters() | ||
}).pipe( |
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.
Since we don't have a setting for languages that autoedits are enabled for, we can use a static list for now without the promiseFactoryToObservable
call.
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
vscode/src/main.ts
Outdated
}) | ||
}), | ||
catchError(error => { | ||
logError('registerAutoedits', 'Error', error) |
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.
Can we use autoeditsOutputChannelLogger
here to ensure this message is added to the autotedits
output channel in the dev environment?
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
vscode/src/configuration.ts
Outdated
@@ -68,7 +74,7 @@ export function getConfiguration( | |||
debugVerbose: config.get<boolean>(CONFIG_KEY.debugVerbose, false), | |||
debugFilter: debugRegex, | |||
telemetryLevel: config.get<'all' | 'off'>(CONFIG_KEY.telemetryLevel, 'all'), | |||
autocomplete: config.get(CONFIG_KEY.autocompleteEnabled, true), | |||
autocomplete: codyAutoSuggestionsMode === CodyAutoSuggestionMode.Autocomplete, |
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.
Should we migrate settings from people with the old configuration field in their settings.json? If I have "cody.autocomplete.enabled": false
in my settings I expect CONFIG_KEY.suggestionsMode
to be false too when I upgrade to the new version. WDYT?
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.
Ah yes, sorry I missed this.
Should we add a manual override here ?
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.
Can we programmatically migrate the current setting value to a new field? For example, if autocomplete.enabled
is set to false
, suggestions.mode
should be manually updated to off
. Also, let's create an issue to follow up and remove this code later.
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.
Added override and create issue: https://linear.app/sourcegraph/issue/CODY-4631/remove-backward-compatibility-override-for-autocomplete-after
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.
Approving to unblock, but let's ensure we address the backward compatibility of the new setting before merging. Or, if I missed anything and it's unnecessary, let me know in the review thread.
Context
Currently
auto-edits
is not reactive to config changes and hence requires vscode reloading. If the user doesn't reload vscodeauto-edits
suggestion doesn't do away because theauto-edits
provider is notdisposed
on the config change.This results into:
auto-edits
suggestions.auto-edits
toautocomplete
mode, they will see the suggestion from bothautocomplete
andauto-edits
.auto-edits
reactive to theauthStatus
andconfig
change similar toautocomplete
, so thatauto-edits
is reactive and destroyed/created on the config changes without reloading vscode.Example image below:
Test plan
dispose
function andprovideInlineCompletion
function forautocomplete
andauto-edits
and make sure they are disposed and created upon config changes without reloading vscode.