-
Notifications
You must be signed in to change notification settings - Fork 1.3k
.NET: Fix CopySessionConfig to respect SessionConfig.Streaming #4797
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
base: main
Are you sure you want to change the base?
Changes from all commits
fb31309
a214f4d
19bcfe6
982793b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -274,8 +274,8 @@ private ResumeSessionConfig CreateResumeConfig() | |
| } | ||
|
|
||
| /// <summary> | ||
| /// Copies all supported properties from a source <see cref="SessionConfig"/> into a new instance | ||
| /// with <see cref="SessionConfig.Streaming"/> set to <c>true</c>. | ||
| /// Copies all supported properties from a source <see cref="SessionConfig"/> into a new instance, | ||
| /// including <see cref="SessionConfig.Streaming"/>. | ||
| /// </summary> | ||
| internal static SessionConfig CopySessionConfig(SessionConfig source) | ||
| { | ||
|
|
@@ -298,13 +298,13 @@ internal static SessionConfig CopySessionConfig(SessionConfig source) | |
| SkillDirectories = source.SkillDirectories, | ||
| DisabledSkills = source.DisabledSkills, | ||
| InfiniteSessions = source.InfiniteSessions, | ||
| Streaming = true | ||
| Streaming = source.Streaming | ||
| }; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Copies all supported properties from a source <see cref="SessionConfig"/> into a new | ||
| /// <see cref="ResumeSessionConfig"/> with <see cref="ResumeSessionConfig.Streaming"/> set to <c>true</c>. | ||
| /// <see cref="ResumeSessionConfig"/>, defaulting <see cref="ResumeSessionConfig.Streaming"/> to <c>true</c> when source is null. | ||
| /// </summary> | ||
| internal static ResumeSessionConfig CopyResumeSessionConfig(SessionConfig? source) | ||
| { | ||
|
|
@@ -327,7 +327,7 @@ internal static ResumeSessionConfig CopyResumeSessionConfig(SessionConfig? sourc | |
| SkillDirectories = source?.SkillDirectories, | ||
| DisabledSkills = source?.DisabledSkills, | ||
| InfiniteSessions = source?.InfiniteSessions, | ||
| Streaming = true | ||
| Streaming = source?.Streaming ?? true | ||
|
||
| }; | ||
| } | ||
|
|
||
|
|
||
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 unit tests for CopySessionConfig currently only assert the default
Streamingvalue; they don’t cover the bug fix scenario wheresource.Streamingis explicitly set tofalse. Please add/adjust tests to verifyStreamingis preserved from the source (including thefalsecase).