-
-
Notifications
You must be signed in to change notification settings - Fork 512
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
Allow custom help providers #1259
Allow custom help providers #1259
Conversation
Version option will show in help even with a default command Reserve `-v` and `--version` as special spectre.console command line arguments (nb. breaking change for spectre.console users who have a default command with a settings class that uses either of these switches) Help writer correctly determines if trailing commands exist and whether to display them as optional or mandatory in the usage statement Ability to control the number of indirect commands to display in the help text when the command itself doesn't have any examples of its own. Defaults to 5 (for backward compatibility) but can be set to any integer or zero to disable completely. Significant increase in unit test coverage for the help writer Minor grammatical improvements to website documentation
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.
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. |
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.
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.
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. |
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. |
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.
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.
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, 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. |
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 I'm sure there's a better way, and certainly more than one way. This is food for thought at least. |
…ather than individual configuration values
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: |
I must have a fundamental misunderstanding, then. I thought the way users added custom help providers was by implementing |
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.
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. |
Ok then we're on the same page, I think. What confuses me is your response to my idea about passing in
Maybe you thought I was talking about a different interface (probably an internal one)? I was only talking about Thinking about the default help provider a little more. Currently, I think you have it working like this (in
Is step 2 necessary? In other words, is there a scenario where if a user does not register a custom implementation of 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 // 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); |
We are on the same page @rcdailey,
However, the bad memories are coming back now... this line This is because the What I think I need to do is add a I'm going to see what this looks like right now... |
Sounds good. I assume by adding it to 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!! |
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 Are you in agreement with this @rcdailey ? |
…gistered in the type factory
…de accessible to Spectre.Console.Cli.Tests)
Code complete & peer-reviewed @patriksvensson, including:
|
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.
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.
…ied Help namespaces throughout codebase; removed use of regions;
…er for reader clarity
…s serves an non-null applicationame when cast as ICommandModel (as used by the help provider)
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.
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?
@FrankRay78 Adding |
Hi @patriksvensson , ok cool. This PR is complete and ready to merge then. |
@FrankRay78 Awesome will take a look at it tonight or 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.
LGTM
Work done
Notes