Closed nothingalike closed 10 months ago
Hello @nothingalike, I implemented the recent implementation of a pluggable HelpProvider and I'm happy to investigate your issue further.
There are currently two unit tests covering an injected TypeRegistrar and a custom HelpProvider, see:
var registrar = new DefaultTypeRegistrar();
// Given
var fixture = new CommandAppTester(registrar);
fixture.Configure(configurator =>
{
// Create the custom help provider
var helpProvider = new CustomHelpProvider(configurator.Settings, "1.0");
// Register the custom help provider instance
registrar.RegisterInstance(typeof(IHelpProvider), helpProvider);
configurator.SetApplicationName("myapp");
configurator.AddCommand<DogCommand>("dog");
});
And
var registrar = new DefaultTypeRegistrar();
// Given
var fixture = new CommandAppTester(registrar);
fixture.Configure(configurator =>
{
// Register the custom help provider type
registrar.Register(typeof(IHelpProvider), typeof(RedirectHelpProvider));
configurator.SetApplicationName("myapp");
configurator.AddCommand<DogCommand>("dog");
});
I'm not sure if you aware of these, and if not, whether they help?
If not, are you able to put an example on a branch/into a gist for me so I can reproduce it locally?
Ah ok. Let me try registering the help provider.
@nothingalike I also wrote some documentation here: https://spectreconsole.net/cli/command-help
It would be good to know if it's fit for purpose, or if you have any suggestions / feedback etc as you go about this.
@FrankRay78 ok so i had some time to test this morning. So far main difference is that you are able to use the DefaultTypeRegistrar
since your tests are running within the same namespace. Nuget consumers do not have that luxury due to the protection level. I made my own TypeRegistrar using the following links:
thoughts?
Hmmm....
There is an example here, where the protection level isn't a problem: https://github.com/spectreconsole/spectre.console/tree/main/examples/Cli/Help
But that doesn't use DI.
There is a DI example here https://github.com/spectreconsole/spectre.console/tree/main/examples/Cli/Injection, with a custom TypeRegistrar/Resolver.
But we don't seem to have an example combining both scenarios. I wonder if that's what's happening in your case?
@FrankRay78 I had a quick glance and I see two "problems" here:
The default IHelpProvider
is always registered, even if a custom IHelpProvider
is already registered.
https://github.com/spectreconsole/spectre.console/blob/5296e56b1c77c6a4312c1c2ac8dbadca099adc03/src/Spectre.Console.Cli/Internal/CommandExecutor.cs#L26-L27
Which probably means that in https://github.com/spectreconsole/spectre.console/blob/5296e56b1c77c6a4312c1c2ac8dbadca099adc03/src/Spectre.Console.Cli/Internal/CommandExecutor.cs#L55
the ?? defaultHelpProvider
is never used, since there's always at least the one IHelpProvider
that was registered in line 27.
"Obviously", our ITypeResolver
uses the first registered instance of a type when multiple registrations are present for a given type. Hitherto, this is a not documented requirement for custom implementations.
Point 1. should be fixable easily enough. Point 2 should be added to TypeRegistrarBaseTests
(and will then most probably break at least all my implementations of an ITypeResolver
😬)
@nothingalike as a quick workaround it should be possible to modify your ITypeResolver
to return the fist registered instance of a given type instead of the last.
@nils-a ok cool, ill give that a shot. thank you
Re: 1 @nils-a, that was a cludge when I found out that our type resolver always returned the first instance. So if you add your own help provider in the configuration, then the lines in Execute is basically pointless. I always suspected some of the DI behaviour would need to be revisited, now is the time.
Ps any reason why the internal spectre.console type registrar shouldn't be publicly available? Then users who use it, should get the same behaviour as our unit tests. Rather than being forced to implement their own to only find undocumented assumptions. I've been thinking about this one for a while.
(Nb. Writing this on iPhone in my STATIONARY car)
You really shouldn't answer GitHub issues while driving 🧐
The point of the DefaultResolver is that it is tied to the DefaultRegistrar. Making those two public would be equivalent to publishing a DI container, the scope of which would be much broader than our own "little" scope.
For everyone unwilling to implement a custom implementations, 3rd party libraries are available. From the top of my head, there's one for Microsoft.Extensions.DependecyInjection and one for SimpleInjector but I guess those are not the only ones.
Ok @nils-a, I've thought about it and I think this issue has fundamentally arisen from the current methods on the DefaultRegistrar, in regards to what happens when a duplicate interface/instance is added.
I had wondered about the following:
AddReplaceInstance()
and AddReplaceType()
so it's clear the add will always replace/supersede if necessaryContains
be added, so users can check if a type already exists, prior to adding oneAddReplaceInstance/Type()
monikersIt was the absence of clarifying above, that led me to add this cludge (on an already massive PR that needed to be drawn to a close, quickly):
var defaultHelpProvider = new HelpProvider(configuration.Settings);
_registrar.RegisterInstance(typeof(IHelpProvider), defaultHelpProvider);
But always blindly registering the internal HelpProvider, without first being able to check if one exists, or understand what should happen when calling register when one already does exist, is how this issue will be ultimately fixed.
Any and all guiding thoughts welcome, and I'm happy to do the coding donkey work.
@FrankRay78 The answer to 1. is: "add". The answers for 2-4 are probably material for a dedicated discussion on the TypeRegistrar/-Resolver system.
For this issue, I would most likely:
IHelpProvider
in the ITypeRegistrar
.IHelpProvider
, only if none could be resolved from the ITypeResolver
.TypeRegistrarBaseTests
:
ISomeInterface
, only the last registration is resolved for Resolve(typeof(ISomeInterface))
ISomeInterface
, all registrations are resolved for Resolve(typeof(IEnumerable<ISomeInterface>))
I am a bit lost. What is the solution (or workaround) to the issue?
That's my code (commented with the errors):
public static class Program
{
public static int Main(string[] args)
{
var services = new ServiceCollection();
services.AddSingleton<IConfigManager>(serviceProvider =>
{
// ...
return new ConfigManager(configLocations);
});
// Error CS0122 : 'TypeRegistrar' is inaccessible due to its protection level
var registrar = new TypeRegistrar();
// Error CS0122 : 'TypeRegistrar.Register<TService, TImplementation>()' is inaccessible due to its protection level
registrar.Register(typeof(IConfigManager), typeof(ConfigManager));
var app = new CommandApp(registrar);
//...
I'm currently using 0.47.1-preview.0.37
(I was hoping it would contain a fix).
Sorry for the noise: I figured it out.
For the future me, see https://github.com/spectreconsole/spectre.console/issues/1285#issuecomment-1717237528
No worries @andreasciamanna, thanks for using our library.
Information
Describe the bug When passing a
TypeRegistrar
to theCommandApp
, it will prevent any customHelpProviders
from being used.To Reproduce
Expected behavior I can use a TypeRegistrar and Custom Help Providers