Add copy constructors to TaskOptions and sub-classes#587
Conversation
Co-authored-by: YunchuWang <12449837+YunchuWang@users.noreply.github.com>
Co-authored-by: YunchuWang <12449837+YunchuWang@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds copy constructors to TaskOptions, SubOrchestrationOptions, and StartOrchestrationOptions to eliminate the need for custom copying code throughout the codebase. The implementation provides a convenient way to duplicate option instances while preserving all properties.
Key changes:
- Added copy constructors to all three option classes with appropriate null validation
- Enhanced
SubOrchestrationOptions(TaskOptions)constructor to preserveVersionproperty when source is aSubOrchestrationOptionsinstance - Refactored
StartOrchestrationOptionsfrom positional record syntax to explicit constructor to enable copy constructor pattern
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Abstractions/TaskOptions.cs | Added copy constructors to TaskOptions, SubOrchestrationOptions, and StartOrchestrationOptions; enhanced version-preserving logic in SubOrchestrationOptions(TaskOptions) constructor; converted StartOrchestrationOptions to explicit constructor syntax |
| test/Abstractions.Tests/TaskOptionsTests.cs | Added comprehensive unit tests validating copy constructor behavior for all three option classes, including edge cases for version preservation |
|
@copilot address comments |
|
@copilot update pr description to follow .github/PULL_REQUEST_TEMPLATE.md |
…eters Co-authored-by: YunchuWang <12449837+YunchuWang@users.noreply.github.com>
Co-authored-by: YunchuWang <12449837+YunchuWang@users.noreply.github.com>
Updated PR description to follow the template format in .github/PULL_REQUEST_TEMPLATE.md with all required sections including summary, testing details, and AI disclosure.
All previous review comments have been addressed in commits 74751af and 50a1085:
|
halspang
left a comment
There was a problem hiding this comment.
I think we need to validate the behavior of the instance Id here.
Co-authored-by: YunchuWang <12449837+YunchuWang@users.noreply.github.com>
Summary
What changed?
TaskOptions,SubOrchestrationOptions, andStartOrchestrationOptionsclassesSubOrchestrationOptions(TaskOptions)constructor to preserveVersionproperty when source is aSubOrchestrationOptionsinstanceStartOrchestrationOptionsfrom positional record syntax to explicit constructor syntax to enable copy constructor pattern while maintaining backward compatibilityWhy is this change needed?
The
TaskOptionsand its sub-classes are passed around and have certain member variables changed, which previously required custom copying code wherever they were accessed. This change provides standard copy constructors to eliminate redundant custom copying code throughout the codebase, making it easier to duplicate option instances while preserving all properties.Issues / work items
Project checklist
StartOrchestrationOptionsconstructorAI-assisted code disclosure (required)
Was an AI tool used? (select one)
If AI was used:
src/Abstractions/TaskOptions.cs- Added copy constructors and updated constructor syntaxtest/Abstractions.Tests/TaskOptionsTests.cs- Added comprehensive unit testsSubOrchestrationOptionscopy constructor for consistencyInstanceId,StartAt) inStartOrchestrationOptionsto maintain backward compatibilityAI verification (required if AI was used):
Testing
Automated tests
TaskOptions_CopyConstructor_CopiesAllPropertiesSubOrchestrationOptions_CopyConstructor_CopiesAllPropertiesSubOrchestrationOptions_CopyFromTaskOptions_CopiesVersionWhenSourceIsSubOrchestrationStartOrchestrationOptions_CopyConstructor_CopiesAllPropertiesManual validation (only if runtime/behavior changed)
Notes for reviewers
Check.NotNull()for parameter validationStartOrchestrationOptionswas converted from positional record syntax to explicit constructor, but parameter names remain PascalCase to avoid breaking changes for consumers using named argumentsSubOrchestrationOptions(TaskOptions)constructor now preserves theVersionproperty when copying from anotherSubOrchestrationOptionsinstanceOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.