CLI Refactor #3460
CLI Refactor #3460
Conversation
- Adds invocation context to pass around lifetime invocation objects
- Splits build/decompile arguments and commands
- Adds service collection extension for dymanic handling of commands
- Refactors main compilation work out into separate service
- Updates/fixes tests
- Adds DI
- adds invocationContext, logger, diagnostics logger and commands to
DI container
- Refactors file writing and console writing out of compilation methods - Adds writers to DI contianer and adds extension to resolve - Removes the unused ICoreArguments interface
|
I've updated the PR ticket with a summary of the main bits that have been cleaned up. I've also removed some duplicated logic from the tests and split the tests to match the CLI structure. After running some perf analysis and it looks like there's been a small amount of performance degradation (~300ms -> ~400ms) for a simple decompile/compile process. This has been minimized wherever I could find it. |
|
thanks @nicktolhurst! Feel free to look through the issues list and let us know if there's something else you'd like to pick up |
| @@ -45,7 +46,7 @@ public void LogSummary() | |||
| this.logger.Log(ToLogLevel(DiagnosticLevel.Info), summary); | |||
| } | |||
|
|
|||
| public int ErrorCount { get; private set; } | |||
| public int ErrorCount { get; set; } | |||
There was a problem hiding this comment.
Good spot! This needs reverting back to a private setter.
| } | ||
| else | ||
| { | ||
| return PathHelper.GetDefaultDecompileOutputPath(inputPath); |
There was a problem hiding this comment.
Yeah, absolutely! Just pushed a commit for that now.
|
Yeah this looks really good! Thanks for doing it! I added a couple of minor questions and comments. Can you take a look? |
…into refactor-3373
Contributing a feature
Fixes #3373
The goal:
To make it easier to add new commands and features to the CLI by simply adding new:
Commands/NewCommand.csArguments/NewArgument.csKeeping in mind that a lot of the implementation may be replaced in future with
System.CommandLine, but the structure can can be very similar, see:Summary:
Unless stated below in (see NOTE:), no implementation logic has changed.
For consistency, move the
--output-dirvalidation to the arguments class.NOTE: This effects the output slightly. Previously, using the
--output-dirflag with a directory that doesn't exist would print the disclaimer before the error message. The ArgumentParser tests that use the--output-dirflag require a valid directory. I've opted to use.in these cases.Split
BuildAndDecompileArgumentsinto separate classes.Root command logic should be handled consistently. E.g.
bicep --helpandbicep --version.Loosen the coupling of services and commands with DI. Commands, services and the two loggers should be added to DI.
Use DI to hold the invocation context (test context, production context, etc).
Move tests around to better match their targets. Since the logic has been refactored into multiple classes, the tests should match these. RootCommandTests, BuildCommandTests, DecompileTests, ProgramTests, etc. In almost all cases the test implementations should be the same.
var (output, error, result) = Bicep("build", "--stdout", bicepFilePath);