Allow custom help providers

[FrankRay78]

Work done

• Implement IHelpProvider
• Port existing HelpWriter to use this new interface
• Remove coupling to internal sealed classes
• Set existing help writer as the default help provider
• Implement a custom help provider to demonstrate functionality and pass unit tests

Notes

1. This PR was branched from Help writer improvements #1252, which seemed like a reasonable starting place.
2. This PR fully replaces Can render command help and application version from inside a command #1133

[rcdailey]

I'll look through this, but the change set is really large (I see 54 files)! If there's any possibility to split the PR to make things easier to review, that would be appreciated, but I will make do with what you have for now.

[rcdailey]

Whew, I got through some of this but there is a lot to go. I need to get back to work but I'll share the comments I have so far. Hopefully I didn't overwhelm you too much. So far, I don't see any issues I feel really strongly about, so please consider all of my comments as unimportant.

Thanks for your effort and giving me the opportunity to review and learn more about the code base.

[FrankRay78]

Thanks for the review @rcdailey, much appreciated. There are a number of things I want to address here, however I'm on holiday for a few weeks shortly and so will pick them up when back mid-Aug. Some of the review issues are hangovers from the existing code base, rather than the introduction of an injectible HelpWriter, however now is probably a good time to consider improving matters.

[rcdailey]

Thanks for the replies. Given how hard you've worked on this and the fact that I haven't really provided any feedback that is terribly important, I don't want to hold you up any more on this PR. There's also a lot to go through and I've had difficulty finding time to resume where I left off.

All that to say: I think what you have is going to be just fine, so if you happen to merge this without any further comments from me, I think that's just fine. Thanks again for all of your hard work.

[rcdailey]

Funny enough, as soon as I leave a comment saying I had no time to review the rest... I ended up making time to do it anyway! Looks like for the most part all I had left to look at was some test code / data files, which was easy. I'll go ahead and give this my approval, for whatever that's worth. Great job!!

EDIT: I'll just add: I think the only thing I have somewhat strong feelings about is my comment from before about mixing DI registrations in the post-initialization / runtime code. I'm sure there are architectural reasons for it, otherwise I imagine you'd have done it differently. We can discuss it in the original comment. I just wanted to put an asterisk next to my approval by reminding you about that one thing.

[FrankRay78]

Hi @rcdailey, I've really appreciated your review comments so far. What's really impressive is actually your last comment about DI, particularly since you aren't 100% familiar with the spectre.console codebase. I've left that review comment until last, picking off the easier ones first, knowing that (in good faith) I should really take the time to consider addressing the DI registration issue (if I can).

You've seen the slightly "bodged" use of how the following line still gets called, _registrar.RegisterInstance(typeof(Help.IHelpProvider), defaultHelpProvider); irrespective of whether a custom IHelpProvider was already registered firstly. When coding this, I discovered the existing DI container allows multiple registrations for the same interface 😱, always resolving to the first one.

I've been thinking about having it throw an exception, or perhaps replacing the existing. It's this I want to spend a bit more time on before having this PR merged.

[rcdailey]

I discovered the existing DI container allows multiple registrations for the same interface 😱, always resolving to the first one

I am not sure which DI container you're referring to, but it probably doesn't matter much since Spectre.Console is agnostic to the DI implementation. For example, I use Autofac, and it actually sets the last registration as the default, not the first. There's a mechanism to override that, though.

Would it make sense to use a configuration property for this instead of letting a user register an implementation "behind your back"?

app.Configure(config =>
{
    config.SetHelpProvider<MyCustomHelp>();
    config.ValidateExamples();
});

This would allow you to conditionally register this type using ITypeRegistrar (either a default, as you currently do, or the user-provided one. And you'd be able to do this at "configure" time instead of in the Run() execution path.

I'm sure there's a better way, and certainly more than one way. This is food for thought at least.

[FrankRay78]

Would it make sense to use a configuration property for this instead

That's a good idea for further consideration, given we are not quite at the stage of wholesale allowing any interface to be overridden and injected. And this PR is focused on specifically allowing custom help providers.

The current DI is custom (before my time), basically to avoid / reduce dependencies on third-party assemblies, see:

[rcdailey]

this PR is focused on specifically allowing custom help providers

I must have a fundamental misunderstanding, then. I thought the way users added custom help providers was by implementing IHelpProvider. What am I missing?

[FrankRay78]

I must have a fundamental misunderstanding, then. I thought the way users added custom help providers was by implementing IHelpProvider. What am I missing?

Sorry, I meant the various internally marked interfaces that spectre.console currently uses. This PR, which is entirely focused on allowing users to extend IHelpProvider, is a beachhead for considering opening up some of the other interfaces for DI, in the fullness of time.

IMO, registrations should happen before application code execution

this becomes important to get right, which I will look at shortly. If i can refactor this in the current PR, I will. Otherwise I'll get the PR merged and the extensible help into main, then come back to do the remaining DI work on a separate PR.

Hope that clears up my thinking. Thanks for chatting this through.

[rcdailey]

Sorry, I meant the various internally marked interfaces that spectre.console currently uses. This PR, which is entirely focused on allowing users to extend IHelpProvider,

Ok then we're on the same page, I think. What confuses me is your response to my idea about passing in IHelpProvider via the configuration object. Your response to that was this:

That's a good idea for further consideration, given we are not quite at the stage of wholesale allowing any interface to be overridden and injected

Maybe you thought I was talking about a different interface (probably an internal one)? I was only talking about IHelpProvider. I just want to make sure we're not talking past each other. I think we're both on the same subject, though.

Thinking about the default help provider a little more. Currently, I think you have it working like this (in CommandExecutor.Execute()):

1. Create a DefaultHelpProvider instance
2. Register that instance with ITypeRegistrar
3. Attempt to resolve IHelpProvider
4. If the resolution returns null, fall back to the DefaultHelpProvider instance created in step

Is step 2 necessary? In other words, is there a scenario where if a user does not register a custom implementation of IHelpProvider that something else outside of the Spectre.Console code base would want to resolve it?

If we can get rid of step 2, then I think that fixes the issue I brought up earlier about a user being potentially unable to override the default help provider implementation due to multiple registrations of IHelpProvider. This would also allow you to avoid constructing a DefaultHelpProvider until you need one:

// Get the registered help provider, falling back to the default provider
// registered above if no custom implementations have been registered.
var helpProvider = resolver.Resolve(typeof(Help.IHelpProvider)) as Help.IHelpProvider 
    ?? new DefaultHelpProvider(configuration.Settings);

[FrankRay78]

We are on the same page @rcdailey,

If the resolution returns null, fall back to the DefaultHelpProvider instance created in step?

However, the bad memories are coming back now... this line resolver.Resolve(typeof(Help.IHelpProvider)) as Help.IHelpProvider actually throws an exception if an IHelpProvider hasn't been registered.

This is because the TypeResolverAdapter class falls back to using the .Net activator, namely return Activator.CreateInstance(type);, which throws because it can't instantiate an interface.

What I think I need to do is add a .Contains(type) method to DefaultTypeRegistrar, so the CommandExecutor can explicitly check if an IHelpProvider has been registered, before deciding whether to call resolver.Resolve(typeof(Help.IHelpProvider)), or alternatively just instantiate the DefaultHelpProvider.

I'm going to see what this looks like right now...

[rcdailey]

Sounds good. I assume by adding it to DefaultTypeRegistrar, you'll also be adding it to the ITypeRegistrar interface? I assume you'll want the ability to verify a registration already exists for custom implementations that users provided (as I mentioned before, I implement it for Autofac).

If that doesn't work out due to the user-observable changes, you could do the below. I don't like it personally, because it sort of toes the line on using exceptions for flow control. But it's better than nothing...

IHelpProvider? helpProvider = null;
try 
{
  helpProvider = resolver.Resolve(typeof(Help.IHelpProvider)) as Help.IHelpProvider;
}
finally
{
  helpProvider ??= new DefaultHelpProvider(configuration.Settings);
}

Whatever you decide, I'm all for it. Just sharing an idea. Thanks again for the hard work!!

[FrankRay78]

I've looked at this and have a heap more uncommitted changes to clean up/refactor/improve the DI.

However, given the size of this PR already, I'm proposing that we leave this one here and seek to get it merged into main. The review has been high-quality and I'm happy with everything checked in currently.

The current way the default help provider is created, then registered, then referenced lower down (which isn't great) is consistent with other patterns in the same method, see red circled below:

I think I would prefer to open a new PR to address the DI-specific refactoring, including looking at splitting the CommandExecutor.Execute() method (see yellow arrow and comment) into something like .FinaliseRegistrations and then .Execute, rather than complicate this PR further.

Are you in agreement with this @rcdailey ?

[patriksvensson]

An initial review of the code. Everything looks good what I can see, but there are some things I think need to be fixed.

One thing is that we probably shouldn't put help stuff in its own namespace unless we want to hide it by default. If we put it in a namespace, we should add Spectre.Console.Cli.Help to Usings.cs so we don't have to explicitly qualify the namespace.

[FrankRay78]

I've fully addressed all reviewer feedback provided.

@patriksvensson There is one last thing that needs to be considered imho, and that's whether the namespace situation is correct. The following image shows all the new interfaces and classes added to expose the help provider implementation:

At the moment, all these interfaces exist to serve the HelpProvider class, hence why they are currently under Spectre.Console.Cli.Help.

They could be left 'as-is' or alternatively, perhaps shifted under Spectre.Console.Cli (ie. promoted) with a view that one day, some other parts of spectre.console might use the same set of fairly generic command model/info interfaces.

nb. adding global using Spectre.Console.Cli.Help; and removing the Help. prefixes from the codebase has already achieved better readability imho.

Do you have a preference in regards to the Help namespace?

[patriksvensson]

@FrankRay78 Adding global using Spectre.Console.Cli.Help and removing the Help qualifier would work since the help stuff only is needed when implementing a help provider.

[FrankRay78]

Hi @patriksvensson , ok cool. This PR is complete and ready to merge then.

[patriksvensson]

@FrankRay78 Awesome will take a look at it tonight or tomorrow 👍

[patriksvensson]

LGTM