Closed adambajguz closed 4 years ago
@adambajguz Thanks for the code contribution. I ran the CliFx.InteractiveModeDemo.dll and did not experience interactivity. In other words, the application showed the help and went back to the command prompt. Is an initial command required to trigger interactivity? By the way, for what it's worth, if @Tyrrrz accepts the pull request, I would be happy to update documentation.
This is explained in readme. You need '[interactive]' directive. This is to ensure you can use applicate as previously, i.e. as a cli tool. There should be also a launch profile in demo project with this already configured. You can also do sth like '[interactive] command'. This will execute command and then show prompt.
Hmm, maybe there should be some configuration to allow startup in interactive mode by default, thus not support tool aka normal mode at all?
Hi @adambajguz. Thanks for the PR.
This is a massive set of changes and, as it stands, it's almost impossible to review properly due to its size. There are too many unrelated changes mixed in, so it would be better to split it up into multiple branches.
Some high level comments:
Like I said in the issue comments, I do not think that adopting interactive mode as part of the core library is a good idea. It's not what the library was designed for and in reality I don't want to maintain that code going forward. I think a much better solution would be to extract that logic to a separate package that enables interactive mode on an ad-hoc basis. Something like adambajguz.CliFx.Interactive
?
ICliExceptionHandler
could be useful, but still need to discuss the design. And it should be in a separate PR.
Manual
same as above.
Not sure about CliContext
, I want to avoid DI at the core of the library and only provide it as an extensibility option for users. What is the use case for this?
WindowWidth
, WindowHeight
, BufferWidth
, and BufferHeight
could be useful for graphics I assume. Also should be in a separate PR.
"Debugger attached to PID {processId}.
not sure why that was required?
I don't want to add dependency on Microsoft.Extensions.DependencyInjection
because DI shouldn't be part of the core and the users should just be able to install whatever container they want.
TableUtils
& TextUtils
could be useful, also need to discuss design in a separate PR.
Hi @Tyrrrz, thanks for comments :)
I'm not sure if this is a good idea to split the changes into multiple branches, as almost all of the changes depend on each other. Making separate branches would mean than I would have to spend another week on copying and adjusting the code.
CliContext
might be useful for debugging, retrieving application configuration and metadata, and crawling all command schemas in application, e.g. someone may wirte a middleware that gives suggestions of commands when the input is wrong. I'm no sure what do you mean by "as an extensibility option for users" since previously all core classes were internal
.Debugger attached to PID {processId}.
is just a message I've added, and as you pointed out it is not required, but for interactive mode it is quite nice (consider the following command: [debug]
- debugger will be attached and every next command will be executed with debugger). After rethinking, maybe a better and more flexible option would removing the message and adding an event. This would allow for some custom user logic, e.g, change logger messages level when debugger is attached, or just show a message.Initially, I didn't want to remove ITypeActivator but I have found that keeping it lowers the flexibility of the framework, and since ASP.NET Core MVC also uses Microsoft.Extensions.DependencyInjection, I don't think this might be a problem. In my opinion, this change makes CliFx to be more a framework than before.
ASP.NET Core MVC allows users to use their DI containers even though everything is based on Microsoft.Extensions.DependencyInjection
, so the same applies for this situation. I do no consider a dependency to Microsoft.Extensions.DependencyInjection
in library that is called a framework as a disadvantage, rather an advantage. The same would apply if Microsoft.Extension.Logging
(ILogger
interface) would be used in the app. The one think that I have forgotten is a method called UseServiceProviderFactory
in CliApplicationBuilder
(maybe sth else also - this definitely requires further investigation).It's not what the library was designed for and in reality I don't want to maintain that code going forward.
there is not a lot of code to maintain - just one class.I think a much better solution would be to extract that logic to a separate package that enables interactive mode on an ad-hoc basis.
- with the current design, especially CliApplicationBuilder class, this is not possible.I will close the PR for now, and reconsider both the changes and Something like adambajguz.CliFx.Interactive?
(maybe since CliFx is MIT-licensed the best option is to publish a new package) ;)
I see, thanks for the reply.
I think design changes should be taken incrementally, in very small steps. A PR with 215 changed files is a no-starter, unfortunately. :)
...with the current design, especially CliApplicationBuilder class, this is not possible.
I would be happy to discuss what should be added in order to make it possible, as it seems to be the better approach. That way the framework would be open for extension and use cases such as yours can benefit from it.
maybe since CliFx is MIT-licensed the best option is to publish a new package
You are also welcome to do that.
For anyone who might be interested what have happened with my changes: >> Typin <<
This pull request introduces a lot of changes, which have their origin in https://github.com/Tyrrrz/CliFx/issues/72 (@jim-wilson-kt). The newly introduced to CliFx interactive mode allows to build applications like mssql-cli or Azure CLI. Since interactive mode is optional and has its implementation in separate class
CliInteractiveApplication
, applications with permanently disabled interactive mode would not have any performance overhead due to the interactive mode.Initially, I didn't want to remove
ITypeActivator
but I have found that keeping it lowers the flexibility of the framework, and since ASP.NET Core MVC also usesMicrosoft.Extensions.DependencyInjection
, I don't think this might be a problem. In my opinion, this change makes CliFx to be more a framework than before.Changes:
CliInteractiveApplication
and interactive only commands.ICliExceptionHandler
andCliApplicationBuilder.UseExceptionHandler(...)
Manual
property inCommandAttribute
that can be used to provide a long, extended description of a commmand.CliContext
that can be injected to services and commands with DI.WindowWidth
,WindowHeight
,BufferWidth
, andBufferHeight
toIConsole
."Debugger attached to PID {processId}.
message after debugger attachment.RootSchema
with HashSet for faster execution, esspecially in interactive mode.CliApplicationBuilder.UseTypeActivator
and added Microsoft.Extensions.DependencyInjection.TableUtils
andTextUtils
.PS. I put a lot of work into making all of the changes. However, since I have introduced a great deal of changes, sth might not be ideal. Feel free to comment or modify!