-
Notifications
You must be signed in to change notification settings - Fork 434
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
Switch to System.CommandLine
#7653
base: master
Are you sure you want to change the base?
Conversation
2f13e8e
to
9c98998
Compare
Can you explain why? I thought Also why change |
@LukaszRozmej We had no particular issues with
|
[ConfigItem(Description = "Enables Hive plugin used for executing Hive Ethereum Tests.", EnvironmentVariable = "NETHERMIND_HIVE_ENABLED", DefaultValue = "false")] | ||
public bool HiveEnabled { get; set; } | ||
//[ConfigItem(Description = "Enables Hive plugin used for executing Hive Ethereum Tests.", EnvironmentVariable = "NETHERMIND_HIVE_ENABLED", DefaultValue = "false")] | ||
//public bool HiveEnabled { get; set; } |
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.
@LukaszRozmej The NETHERMIND_HIVE_ENABLED
seems redundant as we already have NETHERMIND_HIVECONFIG_ENABLED
with IHiveConfig.Enabled
. Related tests are failing, but I don't wanna touch them until I get feedback.
|
||
[ConfigItem(Description = "Sets the job name for metrics monitoring.", EnvironmentVariable = "NETHERMIND_MONITORING_JOB")] | ||
public string MonitoringJob { get; set; } | ||
|
||
[ConfigItem(Description = "Sets the default group name for metrics monitoring.", EnvironmentVariable = "NETHERMIND_MONITORING_GROUP")] | ||
public string MonitoringGroup { get; set; } | ||
//[ConfigItem(Description = "Sets the default group name for metrics monitoring.", EnvironmentVariable = "NETHERMIND_MONITORING_GROUP")] |
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.
@LukaszRozmej NETHERMIND_MONITORING_GROUP
seems no longer relevant.
[ConfigItem(Description = "Plugins directory")] | ||
public string PluginsDirectory { get; set; } | ||
//[ConfigItem(Description = "Plugins directory")] | ||
//public string PluginsDirectory { get; set; } | ||
|
||
[ConfigItem(Description = "Sets the job name for metrics monitoring.", EnvironmentVariable = "NETHERMIND_MONITORING_JOB")] | ||
public string MonitoringJob { get; set; } |
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.
@LukaszRozmej NETHERMIND_MONITORING_JOB
seems no longer relevant.
|
||
void Load(ILogManager logManager); | ||
void Load(); |
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.
The ILogManager
parameter has been removed from the Load()
method. Implementations can require it in their constructors if needed. This was helpful in the context of Program.cs
refactoring.
namespace Nethermind.Api.Extensions | ||
namespace Nethermind.Api.Extensions; | ||
|
||
public class PluginLoader(string pluginPath, IFileSystem fileSystem, ILogger logger, params Type[] embedded) : IPluginLoader |
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.
Added ILogger
to the constructor instead of taking ILogManager
in the Load()
method. That's it for this file.
|
||
public void Load(ILogManager logManager) { } |
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.
Removed ILogManager
from the Load()
method. This implementation doesn't need it at all.
public CancellationToken Token => _cancellationTokenSource!.Token; | ||
public ProcessExitSource(CancellationToken cancellationToken) | ||
{ | ||
_cancellationTokenSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); |
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.
System.CommandLine
provides a cancellation token for ctrl+c
, so we don't need to handle it ourselves. Thus, ProcessExitSource
now uses a linked cancellation token source to combine its own with the one of the CLI parser.
|
||
foreach (var source in _configSource) | ||
// Skip the validation for ArgsConfigSource items as they are already validated by the CLI parser | ||
foreach (IConfigSource source in _configSource.Where(s => s is not ArgsConfigSource)) |
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.
Disabled validation for ArgsConfigSource
(CLI arguments) since the CLI parser does the job now.
Replaces #4310
Changes
McMaster.Extensions.CommandLineUtils
withSystem.CommandLine
as the CLI options parser.cfg
to.json
. The change is backward-compatible, i.e., the.cfg
files are still handled if no.json
equivalent is found. See Remarks below.Program.cs
has been heavily refactored and changed to a top-level filenethermind --configsDirectory path/to/dir --JsonRpc.JwtSecretFile path/to/jwt.hex # or nethermind --configs-dir path/to/dir --jsonrpc-jwtsecretfile path/to/jwt.hex
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Needs manual testing
Documentation
Requires documentation update
PR pending.
Requires explanation in Release Notes
Optionally. Pending.
Remarks
The large number of changed files is due to
.cfg
to.json
rename. I commented on the changes that need attention except forProgram.cs
, which has too many changes. The rest are file scope namespace changes or extension changes.