-
-
Notifications
You must be signed in to change notification settings - Fork 220
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
Advanced CLI #1344
Advanced CLI #1344
Conversation
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.
Actionable comments posted: 8
Outside diff range and nitpick comments (2)
LenovoLegionToolkit.CLI/IpcClient.cs (1)
Line range hint
72-87
: Improve error handling inSendRequestAsync
The method
SendRequestAsync
checks if the pipe exists before attempting a connection, but does not handle the scenario where the server might become unavailable immediately after this check. Consider implementing a retry mechanism or more robust error handling around the pipe connection logic.LenovoLegionToolkit.WPF/CLI/IpcServer.cs (1)
Line range hint
72-127
: Refactor request handling inIpcServer
to improve maintainabilityThe
Handler
method inIpcServer
includes multiple responsibilities, from connection management to request processing. Consider refactoring this method to separate these concerns more clearly, potentially improving the readability and maintainability of the code.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- LenovoLegionToolkit.CLI.Lib/IpcRequest.cs (1 hunks)
- LenovoLegionToolkit.CLI/IpcClient.cs (2 hunks)
- LenovoLegionToolkit.CLI/LenovoLegionToolkit.CLI.csproj (1 hunks)
- LenovoLegionToolkit.CLI/Program.cs (2 hunks)
- LenovoLegionToolkit.WPF/CLI/Features/FeatureRegistration.cs (1 hunks)
- LenovoLegionToolkit.WPF/CLI/Features/FeatureRegistry.cs (1 hunks)
- LenovoLegionToolkit.WPF/CLI/Features/IFeatureRegistration.cs (1 hunks)
- LenovoLegionToolkit.WPF/CLI/IpcServer.cs (4 hunks)
Additional comments not posted (5)
LenovoLegionToolkit.WPF/CLI/Features/IFeatureRegistration.cs (1)
6-13
: InterfaceIFeatureRegistration
ReviewThe methods defined in this interface are essential for the expected functionality of feature handling in the CLI. Each method is asynchronous, which is suitable for potentially long-running operations like checking hardware feature support or retrieving values. The use of generics and asynchronous patterns here is appropriate and aligns well with modern C# practices.
LenovoLegionToolkit.CLI.Lib/IpcRequest.cs (2)
5-12
: Review of EnumOperationType
The
OperationType
enum is well-defined with clear operation types that correspond to the functionalities described in the PR summary. This ensures that the IPC mechanism can distinctly identify and route requests based on the operation type.
14-18
: Review of Properties inIpcRequest
The nullable properties
Operation
,Name
, andValue
are appropriate for the flexibility required in IPC requests, where not all fields may be needed for every operation. This design promotes a versatile and robust IPC request structure.
[APROVED]LenovoLegionToolkit.WPF/CLI/Features/FeatureRegistry.cs (1)
6-28
: Review of Static ClassFeatureRegistry
The
FeatureRegistry
class is well-implemented with a static list that holds all feature registrations. This approach is efficient for accessing and managing feature information across the application. It's important to ensure that all features listed are correctly implemented and integrated within the system.LenovoLegionToolkit.CLI/LenovoLegionToolkit.CLI.csproj (1)
27-29
: Review of Package Reference AdditionThe addition of
System.CommandLine
version2.0.0-beta4.22272.1
is crucial for the CLI enhancements described in the PR. This package will provide robust parsing and handling of command-line arguments, which is essential for the new features being introduced.
After some initial feedback, CLI is being extended with more options. Hence why introduction of a package for handling CMD line arguments.
Scope: