-
Notifications
You must be signed in to change notification settings - Fork 3
feat: add config subcommands (and move to an app builder setup) #166
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
Conversation
…of the complexity and do subcomands because they're unsupported
Do you mean |
|
|
||
| public ParserResult<object> Parse(string[] args) | ||
| { | ||
| //I'm just dealing with one layer of nesting right now. I'm lazy |
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.
It's not infinity until it's two, this is reasonable
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.
Ugh now I'm gonna argue with myself. I do think it's fine that this only handles two for now, but after having looked at it a bit more I think that writing an impl of IHandler that just happened to be a handler and contain the logic for switching across options might actually be simpler than imperatively handling the parent/child relationship (and also give you arbitrarily deep levels for free).
The problem we've, or at least I've, always run against with CLI stuff is how you'd handle passing parent context into child commands. For example say you have a command jib -v peanut -i --lorque lexus -w where jib is the program, -v is a global option, peanut is a subcommand with options -i and --lorque and lexus is a subsubcommand with an option -w. Presumably there's some classes like
class JibOptions { public bool Verbose; }
class PeanutOptions { public bool Internet; public bool Lorque; }
class LexusOptions { public bool Wide; }
and our subcommand type(s) would be something like Thingo<JibOptions, PeanutOptions, LexusOptions>, which sucks.
After seeing the way you use the service collection I started wondering if the entire parsing phase should be doing setup and then the run stage could just pull a type from the service collection that represented the command to run. Then command types could just take their options in the ctor and not actually be generic. The nesting would come cheap because commands with subcommands could just have a hook that sets up the tree subcommands when their name is encountered.
I'm heading to the haunted corn maze in a few mins but I'll try to get an example of what I think that looks like together for tomorrow, or at least be prepared to make one tomorrow.
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.
Yeah I'm not super jazzed on this whole setup either but I didn't want to get down the "Find a new commandline parser that works with ILmerging/AOT and is built to support subcommands.. when you're disappointed that none of them do it then write your own".
I'll be the first to admit that I'm not huge on this solution but I don't want to mess around with finding/writing a new parser. At least right now. - any solution that works within that constraint is fine by me.
That being said I don't think I've really interacted with a lot of programs that have that levels of nesting. IMO other than global options (and in the way this is set up) I think it would be odd to have like peer config -config-option config-option-value show -watch -v. For the globals we could (in this setup) derive child options from the global set.. which again.. less than ideal but at least I don't think it's something we really have to care about with the structure of the app as is. I'm 100% for writing a parser (separately) that doesn't have the same restrictions as CommandLine.net just separate from this.
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.
After seeing the way you use the service collection I started wondering if the entire parsing phase should be doing setup
If we do all of the setup in the parsing phase we kinda end up in the weeds a bit with customizing services based on the config. We could allow registration of handlers (like in the push I've just done for config edit) for post parsing but pre-run config that allow registration of things like filters etc without having to bake it into the primary loop but maybe it's something we can talk about tmrw.
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.
Well I'm going to be late for the thing I'm going to but I think the deriving options generalizes really well overall (again working in the constraints I didn't write the parser that doesn't support subcommands :P):
class JibOptions { public bool Verbose; }
class PeanutOptions : JibOptions { public bool Internet; public bool Lorque; }
class LexusOptions : PeanutOptions { public bool Wide; }
//🐢🐢🐢
Peer/App/App.cs
Outdated
| try | ||
| { | ||
| await result.MapResult<object, Task>( | ||
| x => verb.Handler?.HandleAsync(x, services, token) ?? Task.CompletedTask, |
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.
If verb.Handler is null then the command was invalid right? It would be like doing a command whose only implementation was via subcommands but not specifying a subcommand?
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.
Yeah we should be able to assume at this point that it's never null.
That's exactly it .
config in config (init|show) for ex
|
One thing that I noticed is that we still (based on this structure) can fail out if the config is bad.. but we should allow certain commands to run even if config is busted. I think the solution here is to move the provider registration from eager to on demand - it means duplicated code in a few pathways (getting the provider registration + actioning it) but I think it makes the most sense (I'll do it in the other PR) |
* initial impl of the config edit verb I've included support for EDITOR with a fallback to the default OS provider * formatting pass * classes into their own files * add runtime config per command for registering handlers * pull ProviderLoader from VerbBuilder * cleanup parsing and handle more cases, remove some code from try
So bit of a lazy go at it (and got annoyed with the configuration) so this pr does too much.
It:
peer config init-> Initializes a default config (what we used to echo out)peer config show-> Prints out the current config to the terminalApp config and handlers now look like:
going to finish config edit so that it closes #32