Added functionality to specify default options for commands #736
Conversation
qpooqp
requested review from
davidfowl,
jkotalik,
JunTaoLuo and
rynowak
as code owners
|
|
||
| private static void DefaultOptionsMiddleware(InvocationContext context) | ||
| { | ||
| if (context.ParseResult.CommandResult.Parent != context.ParseResult.RootCommandResult) |
There was a problem hiding this comment.
Is this a common check for these types of middleware? May be worth a comment why you check that.
There was a problem hiding this comment.
Well, not exactly.
I'm checking if the currently executing command is second level (probably not official name but i'm not sure how to name it) command. That means that the command is subcommand of root.
For example:
- Input
tye runis described like thistyeis root commandrunis second level command
- Input
tye buildtyeis root commandbuildis second level command
This check is not necessary at the moment. I was just trying to be safe for the future.
If there will be added third level command and the name of the command will be same as one of the second level (for example tye build run - run is third level and also second level (from tye run) command), then it may add unintended options. So i rather disable it for the moment.
If you know that there will not be third level commands, then i can remove this chcek.
Othervise i will put some comment there.
| return; | ||
| } | ||
|
|
||
| var originalParseResult = context.ParseResult; |
There was a problem hiding this comment.
I think an example here in comments would be useful. Took me a while to understand what was going on here.
There was a problem hiding this comment.
I added some comments to the code, but here if brief description.
- Line 24 - Get value from env-var for current command - if nothing found then exit
- Line 31 - Get all applied options in original input
- Line 32 - Recreate original input
- L 34 - Check if option
--no-defaultis applied in original options - if yes, then exit
Example:tye run --no-default- this will not apply default options - L 39 - Parse default options to validate them
- L 40 - Get valid default options
- L 41 - Filter only the default options which are not already applied in original input
- L 43 - Set new parse result (original input plus default options)
| private const string DefaultOptionsEnvVarPrefix = "TYE_"; | ||
| private const string DefaultOptionsEnvVarPostfix = "_ARGS"; | ||
|
|
||
| private static void DefaultOptionsMiddleware(InvocationContext context) |
There was a problem hiding this comment.
Is it feasible to add a test for this middleware? Is there a mock invocation context you could have to parse?
There was a problem hiding this comment.
I think that i can test it. Will add some tests.
|
@jkotalik Can i have a question about the file The file is in project
|
Go ahead an try moving it, see if anything breaks 😄 . I'm fine to move it. |
|
|
||
| namespace Microsoft.Tye.UnitTests | ||
| { | ||
| public class DefaultOptionsMiddlewareTests |
|
Thanks! |
This is functionality which allows users to specify default options for commands in environment variables and this default options will be automatically applied.
More description with some examples is in #353.