Separate initialization from execution in CommandApp

[rcdailey]

As a user of the API, I'd like the ability to perform global setup and teardown in my application without utilizing base classes for my Command<> / AsyncCommand<> subclasses.

Is your feature request related to a problem? Please describe.

At the moment, if I want to perform some global initialization and/or cleanup, I am required to use base classes or other obscure hacks. The problem with the base class solution is that it creates a lot of constructor boilerplate and makes the code difficult to maintain when changes are needed in that common base class, especially when that change involves modifying dependencies (parameters for DI).

Describe the solution you'd like

I'd like the logic in CommandApp.RunAsync() to be separated such that the following is separated into distinct phases of execution controlled by the user of the API:

1. Parsing of the command tree. Registration of internal types and dependencies. Finally, creation of the implementation for ITypeResolver.
2. Execution of the actual commands.

This allows me to do the following:

1. Perform setup of my command line application as normal.
2. Invoke a new method (or even better: Use the builder pattern) to initiate step 1 above (e.g. CommandApp.Setup()).
3. Access the ITypeResolver via some property (e.g. CommandApp.Resolver) to perform DI resolution for any dependencies needed for my global initialization/cleanup.
4. Perform global initialization as needed in Program.Main()
5. Invoke CommandApp.Run() and store the result.
6. Perform global cleanup as needed in Program.Main()
7. Return the result of the previously executed CommandApp.Run() command.

Describe alternatives you've considered

1. Abusing my implementation of ITypeRegistrar to invoke a callback to perform custom initialization when ITypeRegistrar.Build() is called.
2. Implementing a base class shared by all command handlers that implements Command<> or AsyncCommand<>.

Additional context

I am willing to implement the changes needed here. I've already played around with some ideas in a local branch and landed on two approaches:

1. Quick & dirty: Add new methods to CommandApp with the goal of reducing footprint. Not the most ideal change and am not completely happy with it, but it at least gets that separation I'm aiming for.
2. Implement something like CommandAppBuilder that uses a syntax similar to new CommandAppBuilder().WithConfiguration(...).Build() to construct a CommandApp and return it, which will be responsible for the first phase of initialization. The returned CommandApp would actually be composed in a wrapper object that only allows users to invoke Run() / RunAsync() on it.

The second approach is more drastic, but I think there's a way to maintain backward compatibility in both scenarios.

The reason I'm creating an issue instead of a pull request:

1. I want to discuss the change before I spend time coding it. The change is non-trivial so I need to consider the maintainers' opinions.
2. I'm uncertain about how to split the logic gracefully in CommandExecutor.Execute(). There's a lot of separation of concerns issues in this method:
   • Initialization (registration) logic is mixed in with what appears to be processing logic.
   • There's a circular dependency between DI registration and command tree processing which further complicates separation of the two.
   • There's CLI parsing code for the version options here that probably belongs in its own Command subclass; however I believe cleaning this up properly requires implementing a way to add "global options" to the base command without introducing new subcommands/verbs. There are other issues here that call for this, and I imagine this code exists because of the lack of this functionality.

Happy to help invest time in this library if the developers have the time to help guide me! Thank you for reading!

[FrankRay78]

Hello @rcdailey, thanks for taking the time to write the above. I've thought on several occasions the command setup/parsing could benefit from some rejigging to eliminate some of the statics, make better use of interfaces, support user-provided implementations through the DI somehow.

For example, see this PR: #1133 I've basically encapsulated and exposed the help writer functionality such that users can call it directly from within a command eg. see: https://github.com/spectreconsole/spectre.console/blob/1fba0f97178c5bcf07e0bc51681413a7df199a4f/test/Spectre.Console.Cli.Tests/Data/Commands/DefaultCommand.cs

However, the instantiation of the help writer remains hardcoded instead of being instantiated through DI, currently preventing users from providing their own implementations of IHelpProvider if they wish to do so. I believe what you propose above would fix this but across the entire command lifecycle.

I'm interested in your second approach suggested above. I'm the maintainer for the command line parsing aspect of spectre.console and would be willing to pursue this idea further with you, if you are still interested. I'd also be willing to have a look at any exploratory/POC etc code you may have for this sitting on a branch etc.

[rcdailey]

Hi @FrankRay78, thanks for responding. It's been quite a while since I posted this, so I'll need to go back to my fork and see if I had a local branch or something. I do remember in the process of coding option 2, that I ended up giving up because of the massive amount of refactoring I was doing, which wasn't ideal for backward compatibility concerns. There were also some challenges I couldn't solve really well. I think it had to do with that help/version processing code you mentioned. I think I did have some ideas about how to maintain backward compatibility despite that, but I've long forgotten that by now.

I'm happy to find some time to revisit this and refresh my memory. Will you be able to dedicated time to this over the next few weeks/months? I don't mean for this to sound rude or disrespectful, but I have my own projects I work on during my spare time. Because of the lack of free time and other obligations I have, I don't want to spend more cycles on this if it's going to go cold again for another 7-8 months.

In the meantime, you can see my workaround for global setup and teardown here:

https://github.com/recyclarr/recyclarr/blob/cdf4b2768602f1f4e988d43ee73d14bc097f37c8/src/Recyclarr.Cli/Program.cs#L75-L92

I essentially hacked my type registrar class to let me get access to the internal ILifetimeScope from autofac. This was necessary because ContainerBuilder.Build() doesn't get invoked until I've already called CommandApp.Run(). This is ultimately the problem we're aiming to solve here.

Again I appreciate your response, it means a lot! I hope we can find some solution :)

[FrankRay78]

Hello @rcdailey, thanks for replying. I'm aware of the time lag in this issue and also the difficulty of the issue at hand. I got involved in spectre.console late last year and picked up some of the maintainer role early 2023.

Improving the initialization is of personal/intellectual interest to me, however I have some general project issue/PR housekeeping to do first before getting stuck back into coding (my head isn't in the code at the moment). This will likely take a month of my time before I'm ready to dedicate some thinking to this particular issue.

May I suggest that I drop you a line here when I'm good to go, and we see how you are fixed then? No obligation but this will at least ensure I have some dedicated time to collaborate on this issue.

[rcdailey]

@FrankRay78 I think that's a fantastic idea. I'm of course happy to work on this issue with you, so feel free to ping me here when you're ready. No rush at all. I do have a workaround that keeps me happy for now.

I appreciate your involvement with the project. It means a lot. I'd hate for such a fantastic CLI library to go unmaintained!

[FrankRay78]

Hello @rcdailey, just checking in to say this has not been forgotten.

Rather the work on exposing the help writer needed to be re-implemented to better support DI in CommandExecutor.Execute() (see my comment here: #1224 (reply in thread))

Once done, I think the general usage of DI, and the further potential usage of DI for other, currently internal spectre.console classes, becomes easier and cleaner to do.

[rcdailey]

Thank you for the update! It is very promising that progress is being made that gets us closer to being able to do this. I'm still using my rather unsavory workarounds to have more control over initialization logic. I'm definitely still interested in the outcomes being shared here.

As always, let me know if I can help in any way. You're doing great!

[FrankRay78]

FYI. PR now submitted, if you want a peek at making the help writer injectable and extensible: #1259

[rcdailey]

@FrankRay78 I wanted to circle back to this. It's been a minute, but I recall that the PR you submitted inches us closer to being able to address this issue, but nothing in there directly changes the situation here. Can you confirm is this is correct? And if so, what is the next step to being able to address this?

[FrankRay78]

@FrankRay78 I wanted to circle back to this. It's been a minute, but I recall that the PR you submitted inches us closer to being able to address this issue, but nothing in there directly changes the situation here. Can you confirm is this is correct? And if so, what is the next step to being able to address this?

You are correct. Some other DI improvements did come out of the work, like: #1362

However, the overall work required was a lot more extensive than I expected, so I'm taking a break for a bit.

If no one has picked this up in the meantime, I'll be happy to further progress the DI refactoring when I start again.

[rcdailey]

I'd be happy to work on this for you, if you think we're ready to tackle this. I was always willing to help since I have a personal interest in this issue.

Could you provide some direction/advice as to how we should address this? I'm not as familiar with the current and desired architecture for Spectre.Console. I understand that the DI registration logic needs to be removed from the Run() code path; but not sure the best tactical solution to that. That's where your advice would be beneficial and allow for a more productive code review later.

Thanks for all your hard work!

[FrankRay78]

Hello @rcdailey, just to say that I've pegged your message above, I'm always grateful for contributions, and I will respond more fully soon.

[FrankRay78]

So, I've given this some thought. Basically, Patrik is the overall maintainer for spectre.console and he keeps the architectural decisions in check, It works and I've come to defer to his judgement in these matters. Nils is a co-maintainer who implemented most of the existing DI logic. Given the complexity of this issue, in the end it will come down to both of these individuals deciding the change is worthwhile and on point. That said, they are both open-minded and grateful for good quality contributions, and as the CLI sub-system maintainer, I'm happy to support this once it's at a good standard (ps. many of the currently open, stale PR's were unsolicited, low value, poor quality, didn't have prior support - none of which should happen here).

Secondly, given the complexity of the base class / DI / setup + execute paths you rightly outline, I think perhaps the best way forward is to submit an initial draft PR outlining the indicative changes being proposed, or even just outlining the areas where changes will be needed. Even just inline commentary, kind of like an impact assessment to kick things off. This got me thinking further @rcdailey, if you have a working local version you've used, perhaps you could rebase your existing code to the most recent spectre.console branch, then submit your local changes in a PR for us to take a look at? I could offer an opinion on how that aligns with the rest of the codebase, and it wouldn't take much effort upfront. Alternatively, I am open to other ideas.

PS. This single line is at the heart of the matter, where the register actually gets built:

spectre.console/src/Spectre.Console.Cli/Internal/CommandExecutor.cs

Line 47 in cb52eb6

using (var resolver = new TypeResolverAdapter(_registrar.Build()))

This whole issue is around better managing the pre- and post- actions and behaviour, I believe.

[rcdailey]

Regarding the draft PR idea, I think before I do any further coding, I'd like to see guidance/opinion from @patriksvensson and @nils-a first. I think my initial post here as well as some of the discussion here provide a lot of detail regarding aspects of the problems I observed.

The original coding I did was never complete and IIRC didn't even compile. It was just an exploratory effort. And finally, it was over a year ago since I last touched it so I've forgotten most of what I learned at the time. For all of those reasons, I'd like to discard my original work.

I think it would be best to take a fresh start on this and do it in the proper order this time, by starting with opinions/thoughts from the current maintainers. Hopefully they are willing to provide some thoughts and guidance. If they're OK with my involvement and a particular direction, I'm of course more than happy to do the work and take the initiative on getting a PR through.

I appreciate your help @FrankRay78 and I hope my plan here sounds reasonable.

[FrankRay78]

I discussed this with some of the other maintainers @rcdailey, and there is a feeling that it's currently a solution without a problem. Although several of them did recognise the description of the issue as valid, but equally couldn't recall the exact ways it had been coded around by users of spectre.console. What would be necessary in taking this issue forward, is actual code showing how the current DI implementation becomes unwieldy when used in an app that wants to manage the container life cycle ie. an example of the problem to be fixed. Until then, the issue will remain open as a placeholder waiting for that to come along. At any point, do feel free to submit a coded example, or point out one you may stumble across. There are also other issues, just waiting to be fixed, if you have spare cycles.

[rcdailey]

@FrankRay78 I already have provided an example of how I'm working around the issue in my open source project. The link below is from my 2nd post in this thread:

https://github.com/recyclarr/recyclarr/blob/cdf4b2768602f1f4e988d43ee73d14bc097f37c8/src/Recyclarr.Cli/Program.cs#L75-L92

As you can see, I have to hack my ITypeRegistrar implementation to provide a lambda used to obtain an instance of ILifetimeScope from the ITypeRegistrar.Build() method, which isn't invoked until CommandApp.Run(). This becomes unweildy because I can't decouple my init/deinit logic from Spectre.Console which is an architectural issue. I have wanted to split out logic from my Program.Main for a while now but I can't do that due to the tight coupling the hack requires to CommandApp and other Spectre.Console types.

I should be able to perform operations against my DI container before Run() is invoked. Again, I feel all of this was provided/explained in my initial posts.

It would be great to see direct participation here by the others. I don't see how this is a "solution without a problem" since I have code that clearly demonstrates the struggle I'm dealing with. If there are other approaches, they could pitch in here with those and we can discuss if they are satisfactory.

My response to your initial post might have been a misunderstanding. I thought you were asking for an MCVE or something; which I didn't feel like I needed to provide given 1) the detail I provided in my issue description and 2) the existing open source code example.

[rcdailey]

Also I want to make something super clear. You said this yourself:

couldn't recall the exact ways it had been coded around by users of spectre.console

The problem is exactly that, it's something we have to "code around". It's very clearly a design issue requiring users to do that, if they want to avoid the other workaround pitfalls/issues I mentioned in my OP.

The goal of this issue is to address the design deficiency to such an extent that "coding around" the issue is no longer necessary. It's admittedly a little upsetting that despite my effort, support I've offered so far, and the fact that I've volunteered to do all the coding that this is being referred to as a "solution without a problem" when you yourself said "several of them did recognise the description of the issue as valid".

It doesn't make sense to me how it can be a valid issue but at the same time not recognized as a problem with a solution, especially since I wouldn't bother them to do any of the coding. Surely I must be missing something here.

[FrankRay78]

Apologies for the delay @rcdailey, and also accept my apologies in somehow dropping the ball by not seeing the original Recyclarr repo. I think it was context switching, the number of replies and elapsed time. Anyhow, assuming another maintainer doesn't get there first, I've seen enough in your code to understand enough of the issue now (fingers crossed), and would like to look into fixing this somehow. Time permitting, this will likely be over the Christmas holiday period. Thanks for your patience on this one.

[rcdailey]

Thank you Frank, and I apologize that I let my frustration leak through. Happy Holidays to you.

[FrankRay78]

Thank you Frank, and I apologize that I let my frustration leak through. Happy Holidays to you.

It's fine, no apology necessary. But appreciated. So much OSS goes nowhere, for lack of involvement or poor communication or a host of other reasons. It's good you care enough to push this along, that's a massively good indicator.

[nils-a]

@rcdailey would you have the cycles to take a look at #1412? It may solve your requirement.

[FrankRay78]

Fixed by #1412.