-
-
Notifications
You must be signed in to change notification settings - Fork 278
Allow Command to be subclassed by application code #288
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
Conversation
…ocessEx to return Stream.Null when not redirected
…to allow specialization
|
Thanks @DaRosenberg. |
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.
Pull Request Overview
This pull request enables subclassing of the Command class by refactoring the configuration API to use an immutable CommandConfiguration struct while still supporting reconfiguration on the Command instance.
- Refactors the fluent configuration API to delegate to a contained immutable CommandConfiguration instance.
- Adds additional constructors and xmldocs on Command to support subclassing.
- Updates ProcessEx and Command.Execution to use configuration snapshots to protect against concurrent modifications.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| CliWrap/Utils/ProcessEx.cs | Updates Standard* properties to return Stream.Null when not redirected. |
| CliWrap/CommandConfiguration.cs | Introduces the immutable CommandConfiguration record struct and its overload constructor. |
| CliWrap/Command.cs | Refactors the Command API to update the contained configuration in place and exposes ICommandConfiguration. |
| CliWrap/Command.Execution.cs | Updates process start info construction by snapshoting configuration and marking CreateStartInfo as protected virtual. |
| CliWrap.Tests/ValidationSpecs.cs | Adjusts tests to validate against Command.Configuration rather than Command directly. |
| CliWrap.Tests/ConfigurationSpecs.cs | Updates tests to assert that the same Command instance is mutated by the reconfiguration API. |
Comments suppressed due to low confidence (1)
CliWrap/Command.cs:112
- The fluent API methods now mutate the command instance in place instead of returning a new instance, which deviates from a typical immutable design. Please confirm that the mutable behavior is intentional and that any thread-safety implications are clearly documented.
public Command WithTargetFile(string targetFilePath)
|
I'll add my comments back in the original issue because there's more context to the discussion there |
Summary of changes:
Commandinheritable by changing the implementation of the fluent configuration APICommandinstances, the fluent configuration API now delegates to a contained immutableCommandConfigurationstruct. This preserves the immutability and atomicity of the configuration (and the resulting thread safety) while allowing theCommandobject to be reconfigured.Commandand clarifies their xmldocsCommandConfigurationis always copied to a local variable up-front to protect against any concurrent modificationsCommand.CreateStartInfo()fromprivatetoprotected virtualto allow derived classes to customize how theProcessStartInfois configuredStandardInput,StandardOutput, andStandardErrorproperties onProcessExto returnStream.Nullwhen not redirectedTo avoid any breaking changes,
Commandstill implementsICommandConfigurationbut now delegates the implementation to the containedCommandConfigurationinstance. This should be removed in next major, and applications should instead use the newCommand.Configurationproperty.Closes #79