Conversation
skdeng
left a comment
There was a problem hiding this comment.
I haven't finished looking through AzSuggestion.cs file, will finish later
|
|
||
| namespace Microsoft.PowerShell | ||
| { | ||
| public partial class PSConsoleReadLine |
There was a problem hiding this comment.
question: Is there a reason why the name of the class is the not the same as the name of the file?
There was a problem hiding this comment.
This seems to be the convention for much of PSReadLine, e.g. History.cs is also part of the PSConsoleReadLine class (so is Completion.cs, etc.)
| const char commandSplitTokens = '|'; | ||
| const string serviceUri = "http://localhost:3000/prediction"; // TODO use real service URI | ||
| HttpClient client = new HttpClient(); | ||
| List<string> suggestions = new List<string>(); | ||
| List<string> commands = new List<string>(); | ||
| HashSet<string> commandSet = new HashSet<string>(); | ||
| List<Dictionary<string, string>> logs = new List<Dictionary<string, string>>(); |
There was a problem hiding this comment.
use var instead of explicit types when the variable type is unambiguous
There was a problem hiding this comment.
I have updated code in methods for this; I think these top level declarations must be explicitly typed
| // If no history available, use start token | ||
| if (i < 0) | ||
| { | ||
| previousLines.Add("start_of_snippet"); |
There was a problem hiding this comment.
if _history is empty, two "start_of_snippet" will be added. is this the intended behavior?
There was a problem hiding this comment.
Yes, the lookup model in the service is expecting n = 2 lines of history and start_of_snippet is the token for "no history"
…where possible, regex match Azure commands.
mirdaki
left a comment
There was a problem hiding this comment.
Looks pretty good. Mostly just nits from me.
| { "CorrelationId", "00000000-0000-0000-0000-000000000000" }, | ||
| { "SessionId", "00000000-0000-0000-0000-000000000000" }, | ||
| { "SubscriptionId", "00000000-0000-0000-0000-000000000000" }, | ||
| { "VersionNumber", "1.0" } |
There was a problem hiding this comment.
Are there meaningful values for these we can use? If not, aside from the version number, they are not required to be sent.
There was a problem hiding this comment.
Right, this is a work in progress! Trying to locate the telemetry package that can provide these.
| commands = JsonConvert.DeserializeObject<List<string>>(reply); | ||
| commandSet = new HashSet<string>(commands.Select(x => x.ToLower())); | ||
| waitForCommands = false; | ||
| RequestPredictions(); |
There was a problem hiding this comment.
Is there any situation where you would want the commands, but not need to also request the predictions? It might make sense to break them down more.
There was a problem hiding this comment.
Since commands are only fetched once on session startup (and fetching the commands as a concept is essentially a stopgap until a better model is created), I think they are pretty well entangled.
Add telemetry tracking Use parameter bagging for order independent prediction, move query structure to new file, add telemetry client and logging capabilities
Summary
Add request calls to a prediction service endpoint that return suggestions for Azure PowerShell commands. These suggestions are offered in addition to history suggestions when user is typing in console.
To provide this functionality, minimal changes are made to existing logic, and a new file AzSuggestion.cs is added to handle the requests, processing and logging.
PR Checklist