Move generic code to strategies library. #5972
Conversation
|
|
||
| public static string UpdateLocation( | ||
| this IState current, string location, IResourceConfig config) | ||
| => location ?? current.GetLocation(config) ?? "eastus"; |
There was a problem hiding this comment.
do we always want to default to eastus for all usages of the strategies library?
There was a problem hiding this comment.
I'm not sure. We need to discuss this.
| { | ||
| // create a DAG of configs. | ||
| var config = await parameters.CreateConfigAsync(); | ||
|
|
There was a problem hiding this comment.
remove extra vertical space between lines (only vertical space after ending }
| { | ||
| return string.Join(",", e.Cast<object>().Select(ToPowerShellString)); | ||
| } | ||
| return value.ToString(); |
| var config = taskProgress.Config; | ||
| activeTasks.Add(config.GetFullName()); | ||
| } | ||
| progress += taskProgress.GetProgress(); |
There was a problem hiding this comment.
vertical space after '}' (please apply throughout file)
| { | ||
| public static class AsyncCmdletExtensions | ||
| { | ||
| public static void WriteVerbose(this IAsyncCmdlet cmdlet, string message, params object[] p) |
There was a problem hiding this comment.
Please provide documentation comments for this class, and for all public properties and methoids (you have most of the methods)
| { | ||
| interface IAsyncCmdlet | ||
| public interface IAsyncCmdlet |
There was a problem hiding this comment.
Please provide documentation comments for this public interface and each of its methods and properties.
| { | ||
| public interface ICmdlet | ||
| { | ||
| void WriteVerbose(string message); |
There was a problem hiding this comment.
WriteVerbose, WriteObject, and WriteProgress are already in ICommandRuntime. Not sure why we need this - seems like just for Parameters.
Also, please provide documentation comemnts on the interface and properties.
There was a problem hiding this comment.
I would like to use ICommandRuntime if I can use it in the Strategy library without introducing dependencies on PowerShell.
| { | ||
| internal sealed class ShouldProcess : IShouldProcess | ||
| public interface IParameters<TModel> |
There was a problem hiding this comment.
Can we retroactively add documentation comments here?
…-shandar/azure-powershell into sergey-strategies-update2
| return newState.Get(config) ?? current.Get(config); | ||
| } | ||
|
|
||
| static string UpdateLocation( |
There was a problem hiding this comment.
I don't think that the default should be east us, I think this is something that should be set for each client (perhaps a DefaultLocation property.
The fear is that services that are not available in eastus will default to this value inadvertently
Description
Depends on #5933
Checklist
CONTRIBUTING.mdplatyPSmodule