Fix/4130 disable color output for cli #4785
Conversation
|
|
||
| private static readonly CliModuleLoader ModuleLoader = new CliModuleLoader(Engine, NodeManager, CliConsole); | ||
| var colorSchemeOption = app.Option("-cs|--colorScheme <colorScheme>", "Color Scheme. Possible values: Basic|Dracula", CommandOptionType.SingleValue); |
There was a problem hiding this comment.
NP: cliColorScheme instead of colorScheme.
There was a problem hiding this comment.
Nevermind, this is for CLI anyway.
| ? new ColorfulCliConsole(cs) | ||
| : new CliConsole(); | ||
|
|
||
| var historyManager = new StatementHistoryManager(cliConsole); |
There was a problem hiding this comment.
NP: Inconsistent use of var or explicit type. I personally prefer var, but intellij seems to like explicit type better, which seems to be the main convention in Nethermind. In any case, please be consistent within the same file.
There was a problem hiding this comment.
Sure, I'm used to using var, but I'll adjust to convention.
| } | ||
|
|
||
| private static void TestConnection() | ||
| private static void TestConnection(INodeManager nodeManager, ICliEngine cliEngine, ICliConsole cliConsole) |
There was a problem hiding this comment.
NP: please refrain from refactoring at the same time. Side note, I'd prefer previous style in this case.
There was a problem hiding this comment.
Not sure what you mean in this instance - is it the use od static fields instead of method params? If so, I feel this is a part of the change as the cliConsole and items which depend on it are now derived during runtime.
There was a problem hiding this comment.
You can also change just the static CliConsole and that would require less change. Of course, static var here is also not very nice. In any case, its not a breaking change.
…lor_output_for_Cli
Closes #4130
Changes:
Types of changes
What types of changes does your code introduce?
Put an
xin the boxes that applyTesting
Requires testing
In case you checked yes, did you write tests??
Comments about testing , should you have some (optional)
Tested manually using Windows cmd, Windows terminal and bash on Ubuntu.
Further comments (optional)
It's worth to note that using different terminals may result in applying different colors when using colorScheme parameter. This is due to specific .Net console implementation (indexed colors) as well as Colorful.Console package implementation trying to find closest color in RGB space. None of these exiting behaviours is modified as a part of this change, but the ability to disable color output by default should help when you encounter issues using your favourite terminal.