#2708 add Add a judgment to the graph constructor #2709
Conversation
|
@microsoft-github-policy-service agree |
| namespace AutoGen.Core.Tests | ||
| { | ||
| [TestClass()] | ||
| public class GraphTests |
There was a problem hiding this comment.
Could you move this test to AutoGen.Tests project? That's where we usually put AutoGen.Core's tests
| this.transitions.AddRange(transitions); | ||
| } | ||
|
|
||
| public Graph(IEnumerable<Transition>? transitions) |
There was a problem hiding this comment.
Why transitions can be null in this case. Would it be better to simply does a null check and throw an exception when transitions is null in this constructor?
There was a problem hiding this comment.
In some cases the external developer may pass in a null List Object. This is allowed at least at compile time, so it's a risk. Because in some cases you don't need to set up the transition when creating Graph entity, but dynamically configure it later, there is no need to create a new object in this case.
| @@ -35,12 +35,13 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "AutoGen.Mistral.Tests", "te | |||
| EndProject | |||
| Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "AutoGen.SemanticKernel.Tests", "test\AutoGen.SemanticKernel.Tests\AutoGen.SemanticKernel.Tests.csproj", "{1DFABC4A-8458-4875-8DCB-59F3802DAC65}" | |||
There was a problem hiding this comment.
Why these changes are added
There was a problem hiding this comment.
I think it's added by VS automatically. I haven't add any new project to the sln after removed the Core.Test project
There was a problem hiding this comment.
Can you revert this change
git checkout main dotnet/AutoGen.sln
Why are these changes needed?
Related issue number
#2708
Checks