-
Notifications
You must be signed in to change notification settings - Fork 8
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
Autocomplete #166
Autocomplete #166
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.
Haven't tested it locally but the code lgtm. Approve to unblock
//var result = agentService.GetWorkspaceDocuments(new GetDocumentsParams { Uris = new[] { fullPath.ToUri() } }).Result; | ||
//trace.TraceEvent("AfterDidChange", result.Documents.First().Content); |
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.
//var result = agentService.GetWorkspaceDocuments(new GetDocumentsParams { Uris = new[] { fullPath.ToUri() } }).Result; | |
//trace.TraceEvent("AfterDidChange", result.Documents.First().Content); |
Left over?
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.
Add unit tests for both methods.
@@ -30,6 +30,7 @@ public async Task Solution_Name_Is_Added_To_Chat_Input() | |||
await OpenSolution(SolutionsPaths.GetConsoleApp1File("ConsoleApp1.sln")); | |||
|
|||
// when | |||
await ShowChatTab(); |
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.
Redundant call to ShowChatTab()
- its called in the GetChatContextTags()
method.
@@ -77,6 +77,7 @@ private async Task NotInLoggedState(Func<Task> action) | |||
finally | |||
{ | |||
await RevertToken(); | |||
await Task.Delay(1000); |
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.
Try removing the delay and check in the CI. It should work without it.
|
||
WriteLog($"Opening solution '{path}' ..."); | ||
Dte.Solution.Open(path); | ||
|
||
await Task.Delay(TimeSpan.FromSeconds(5)); | ||
WriteLog("Delay after solution open stopped."); | ||
await tcs.Task; |
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 refactor and removing Task.Delay()
from here.
@@ -55,10 +55,10 @@ public sealed partial class CodyPackage : AsyncPackage | |||
public ILog AgentLogger; | |||
public ILog AgentNotificationsLogger; | |||
|
|||
public static IAgentService AgentService; | |||
public static IUserSettingsService UserSettingsService; |
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.
Is there any specific reason why static
is used for UserSettingsService
?
private static TraceLogger trace = new TraceLogger(nameof(CodyProposalSourceProvider)); | ||
|
||
private readonly ITextDocumentFactoryService textDocumentFactoryService; | ||
private readonly IVsEditorAdaptersFactoryService editorAdaptersFactoryService; |
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.
Looks like editorAdaptersFactoryService is not used anywhere.
|
||
namespace Cody.VisualStudio.Services | ||
{ | ||
public class TestingSupportService |
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.
In which scenario this class is used and how? Looks like SetAutocompleteSuggestion()
is only called but not GetAutocompleteSuggestion()
.
caret = caret.TranslateTo(trackedSnapshot); | ||
if (completionState != null) | ||
{ | ||
completionState = completionState.TranslateTo(trackedSnapshot); |
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.
trackedSnapshot
could be null. Shouldn't if (completionState != null)
also check trackedSnapshot != null
?
if (textView != null) | ||
{ | ||
var wpfTextView = editorAdaptersFactoryService.GetWpfTextView(textView); | ||
snapshot = snapshot ?? wpfTextView.TextSnapshot; |
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.
Shouldn't wpfTextView
be checked if not null?
Tab
keyboard press (lalt+,
for rotating suggestions if there is more than one)lalt+.
)Test plan
Tab
Esc