WORK: add new Interface to support TreeNodeContext in Parameters.Call… #32
Conversation
| { | ||
| if (sessionId == null) throw new ArgumentNullException("sessionId"); | ||
| if (string.IsNullOrWhiteSpace(treeNodeKey)) throw new ArgumentNullException("treeNodeKey"); | ||
| if (userContext == null) throw new ArgumentNullException("userContext"); |
There was a problem hiding this comment.
userContext [](start = 16, length = 11)
userContext can be null. It's an optional parameter. #Closed
There was a problem hiding this comment.
removed the null check, and added the comment
userContext is an optional parameter for TreeNode, so we don't throw ArgumentNullException when null.
In reply to: 504159484 [](ancestors = 504159484)
| // Evaluate the dynamic properties that are used by the actionTask. | ||
| treeNodeContext = new TreeNodeContext( | ||
| this.Parameters.SessionId, | ||
| treeNodeKey, |
There was a problem hiding this comment.
treeNodeKey [](start = 28, length = 11)
current instead of treeNodeKey #Closed
There was a problem hiding this comment.
| if (this.Parameters.CallbacksV2 != null) | ||
| { | ||
| // If applicable, use the new CallbacksV2 with ITreeWalkerCallbacksV2 | ||
| // Evaluate the dynamic properties that are used by the actionTask. |
There was a problem hiding this comment.
// Evaluate the dynamic properties that are used by the actionTask. [](start = 24, length = 67)
Remove this comment. #Closed
There was a problem hiding this comment.
| await this.Parameters.Callbacks.AfterVisitNode( | ||
| if (treeNodeContext != null) | ||
| { | ||
| // Follow the previous Callbacks with ITreeWalkerCallbacks. |
There was a problem hiding this comment.
// Follow the previous Callbacks with ITreeWalkerCallbacks. [](start = 28, length = 59)
Comment does not line up with V2 scenario. #Closed
There was a problem hiding this comment.
| if (treeNodeContext != null) | ||
| { | ||
| // Follow the previous Callbacks with ITreeWalkerCallbacks. | ||
| await this.Parameters.CallbacksV2.AfterVisitNode(treeNodeContext).ConfigureAwait(false); |
There was a problem hiding this comment.
treeNodeContext [](start = 77, length = 15)
Hmm.. Let's discuss re-using the same TreeNodeContext object for BeforeVisitNode and AfterVisitNode. It is the same object, so it makes sense to use the same object. However, the existing behavior recalculates the Properties value. The user could potentially be relying on Properties getting calculated twice. Or they could be assuming it should only be calculated once.. Not sure which way to go here. #Closed
There was a problem hiding this comment.
Another thing to consider is that BeforeVisitNode will be manipulating property values of TreeNodeContext, such as the new SkipAction property. Given that case, we would need to pass the same object to AfterVisitNode.
In reply to: 504167224 [](ancestors = 504167224)
There was a problem hiding this comment.
agree with you on both. So i now changed it to use different objects, for the recalculating, and copy the value of ShouldSkipActionsInTreeNode
In reply to: 504228812 [](ancestors = 504228812,504167224)
There was a problem hiding this comment.
Ok sounds good. Doesn't look like you set currentNodeSkipActionContext on AfterVisitNode's TreeNodeContext. This should be set to the value from BeforeVistNode's TreeNodeContext.
In reply to: 504291737 [](ancestors = 504291737,504228812,504167224)
There was a problem hiding this comment.
| /// <exception cref="TimeoutException">If the node-level timeout was hit.</exception> | ||
| /// <exception cref="ActionTimeoutException">If the action-level timeout was hit.</exception> | ||
| /// <exception cref="OperationCanceledException">If the cancellation token was triggered.</exception> | ||
| /// <exception cref="Exception">If an unexpected exception was thrown.</exception> | ||
| /// <returns>The key of the next child to visit, or <c>null</c> if no match was found.</returns> | ||
| public async Task<string> VisitNode(string treeNodeKey) | ||
| { | ||
| return await this.VisitNode(treeNodeKey, skipAction: false); |
There was a problem hiding this comment.
await [](start = 19, length = 5)
An optimization with async/await when the method simply calls another async/await method. You can instead just return the method and remove the async/await.
public Task VisitNode(string treeNodeKey)
{
return this.VisitNode(treeNodeKey, skipAction: false);
}
Everything works the same but you save some overhead of dealing with another async await. #Closed
There was a problem hiding this comment.
good tips, and it can save one more context switch by saving one async await.
In reply to: 504169506 [](ancestors = 504169506)
| @@ -406,43 +438,66 @@ public async Task<string> WalkTree(string treeNodeKey) | |||
| /// Visits a TreeNode in the ForgeTree, performing type-specific behavior as necessary before selecting the next child to visit. | |||
| /// </summary> | |||
| /// <param name="treeNodeKey">The TreeNode key to visit.</param> | |||
| /// <param name="skipAction">Specifies whether to skip all actions and do ChildSelector only.</param> | |||
There was a problem hiding this comment.
/// Specifies whether to skip all actions and do ChildSelector only. [](start = 8, length = 101)
This method does not have the skipAction param. Remove the tag. #Closed
There was a problem hiding this comment.
There was a problem hiding this comment.
I still see the SkipAction param in iteration 5.
In reply to: 504295201 [](ancestors = 504295201,504169707)
There was a problem hiding this comment.
| @@ -0,0 +1,1143 @@ | |||
| //----------------------------------------------------------------------- | |||
| // <copyright file="TreeWalkerUnitTestsWithV1Callbacks.cs" company="Microsoft"> | |||
There was a problem hiding this comment.
TreeWalkerUnitTestsWithV1Callbacks [](start = 20, length = 34)
We should be able to test the new behavior in a few tests without having to duplicate the entire test file for this one change. #Closed
There was a problem hiding this comment.
yes, we should be able to update a few tests. My thought is to use a copied file to cover all existing tests with V1, so V1 is fully verified. Then update the existing tests with V2, so V2 are fully covered. And we can delete "TreeWalkerUnitTestsWithV1Callbacks.cs" in future when we officially retire V1 interface.
In reply to: 504215906 [](ancestors = 504215906)
| this.sessionId, | ||
| jsonSchema, | ||
| this.forgeState, | ||
| callbacksV1, // Passes in ITreeWalkerCallbacks as V1, expects TreeWalkerParameters will recognize the instance is actually V2 and uses it as V2; otherwise throw NotImplementedException |
There was a problem hiding this comment.
// Passes in ITreeWalkerCallbacks [](start = 40, length = 34)
This comment is too long to be in-line. Please move it to its own line, possibly multi-line. #Closed
There was a problem hiding this comment.
| this.callbacks = new TreeWalkerCallbacks(); | ||
| TreeWalkerCallbacksV2 treeWalkerCallbacksV2 = new TreeWalkerCallbacksV2(); | ||
| treeWalkerCallbacksV2.ShouldSkipActionsInTreeNode = shouldSkipActionsInTreeNode; | ||
| this.callbacksV2 = treeWalkerCallbacksV2; |
There was a problem hiding this comment.
I think a single line is cleaner here.
this.callbacksV2 = new TreeWalkerCallbacksV2() { ShouldSkipActionsInTreeNode: shouldSkipActionsInTreeNode}; #Closed
There was a problem hiding this comment.
totally agree, changed to
this.callbacksV2 = new TreeWalkerCallbacksV2() { ShouldSkipActionsInTreeNode = shouldSkipActionsInTreeNode };
In reply to: 504218674 [](ancestors = 504218674)
| { | ||
| // Leaf type can't have ChildSelector so we return here. | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Why did you move this? #Closed
There was a problem hiding this comment.
because regardless skipAction is true or false, we need the check below. So i moved the code to the bottom.
if (treeNode.Type == TreeNodeType.Leaf)
{
// Leaf type can't have ChildSelector so we return here.
return null;
}
In reply to: 504218989 [](ancestors = 504218989)
| { | ||
| using System; | ||
| using System.Threading; | ||
| using System.Threading.Tasks; |
There was a problem hiding this comment.
Can we remove these? I don't see any Tasks in this file. #Closed
There was a problem hiding this comment.
I was thinking if SkipAction property is true, how does the schema know that happened? Schema must know this happened so it can respond accordingly in the ChildSelector. Here are a few ideas Option 1, persist new state. Option 2, set the ActionResponse with Skipped Status and check for that. Option3, OnBeforeVisitNode has new property specifying the desired Child to go to. Maybe this gets persisted? Refers to: Forge.TreeWalker/src/TreeWalkerSession.cs:800 in 991a747. [](commit_id = 991a747, deletion_comment = False) |
Option 2, we need to commit the LastAction while all actions in this node are skipped. not sure how to do it. Option 1, we need to set Session.SkippedCurrentTreeNode to true before selection, and set the value to false after selection. Option 3, limit the FallbackChild to only one node, while we should allow multiple choices. let me try option 1 In reply to: 707985867 [](ancestors = 707985867) Refers to: Forge.TreeWalker/src/TreeWalkerSession.cs:800 in 991a747. [](commit_id = 991a747, deletion_comment = False) |
….CurrentNodeSkipActionContext
…deSkipActionContext()
| Guid rootSessionId, | ||
| string currentNodeSkipActionContext) | ||
| { | ||
| // userContext is an optional parameter for TreeNode, so we don't throw ArgumentNullException when null. |
There was a problem hiding this comment.
// userContext is an optional parameter for TreeNode, so we don't throw ArgumentNullException when null. [](start = 12, length = 104)
Please remove this comment. UserContext being an optional parameter is established in TreeWalkerParameters as well as on the wiki. We don't need to remind folks in more places of this fact. #Closed
There was a problem hiding this comment.
|
|
||
| // Follow the previous Callbacks with ITreeWalkerCallbacks. | ||
| await this.Parameters.CallbacksV2.BeforeVisitNode(treeNodeContext_BeforeVisitNode).ConfigureAwait(false); | ||
| this.currentNodeSkipActionContext = treeNodeContext_BeforeVisitNode?.CurrentNodeSkipActionContext; |
There was a problem hiding this comment.
treeNodeContext_BeforeVisitNode? [](start = 60, length = 32)
Are you null checking here in case BeforeVisitNode sets TreeNodeContext to null for some reason? This may be overly safe, but I'm okay with it. #Closed
There was a problem hiding this comment.
| @@ -321,18 +358,39 @@ public async Task<string> WalkTree(string treeNodeKey) | |||
| } | |||
| finally | |||
| { | |||
| // Always call this callback, whether or not visit node was successful. | |||
| await this.Parameters.Callbacks.AfterVisitNode( | |||
| if (treeNodeContext_BeforeVisitNode != null) | |||
There was a problem hiding this comment.
if (treeNodeContext_BeforeVisitNode != null) [](start = 24, length = 44)
If we're just using treeNodeContext_BeforeVisitNode for this null check, I'd prefer to just use the same check here:
if (this.Parameters.CallbacksV2 != null)
Then we don't have to use 2 different names for TreeNodeContext variable. Since they will both live inside separate if statements, we can call them both treeNodeContext. #Closed
There was a problem hiding this comment.
… and V2 interface
| this.SessionId = sessionId; | ||
| this.JsonSchema = jsonSchema; | ||
| this.ForgeState = forgeState; | ||
| this.CallbacksV2 = callbacksV2; | ||
| this.Token = token; | ||
| } |
There was a problem hiding this comment.
Let's remove this constructor. I want folks to use ForgeTree instead of jsonSchema. Introducing a new V2 interface gives a good opportunity to phase jsonSchema out as well. #Closed
There was a problem hiding this comment.
| /// The ITreeWalkerCallbacks interface defines the callback Tasks that are awaited while walking the tree. | ||
| /// Prefers future callers to use the new ITreeWalkerCallbacksV2 CallbacksV2, and keep this ITreeWalkerCallbacks Callbacks for backward compatibility. |
There was a problem hiding this comment.
, and keep this ITreeWalkerCallbacks Callbacks for backward compatibility [](start = 84, length = 73)
Remove this part. #Closed
There was a problem hiding this comment.
| await this.Parameters.Callbacks.AfterVisitNode( | ||
| if (this.Parameters.CallbacksV2 != null) | ||
| { | ||
| // When CallbacksV2 is not null, continues with the new ITreeWalkerCallbacksV2. |
There was a problem hiding this comment.
// When CallbacksV2 is not null, continues with the new ITreeWalkerCallbacksV2. [](start = 28, length = 79)
Let's use the same comment in BeforeVisitNode and AfterVisitNode. #Closed
There was a problem hiding this comment.
There was a problem hiding this comment.
fixed => // If applicable, use the new CallbacksV2 with ITreeWalkerCallbacksV2
In reply to: 505917661 [](ancestors = 505917661,505912854)
| if (this.Parameters.CallbacksV2 != null) | ||
| { | ||
| // When CallbacksV2 is not null, continues with the new ITreeWalkerCallbacksV2. | ||
| treeNodeContext = new TreeNodeContext( |
There was a problem hiding this comment.
treeNodeContext = new TreeNodeContext( [](start = 28, length = 38)
Let's add a comment here since its not obvious why we are recreating the TreeNodeContext.
// Recreating the TreeNodeContext here to reevaluate TreeNode.Properties. #Closed
| /// Get the string context if the actions in this TreeNodes are skipped. | ||
| /// Usually, when there is no action skipped, the return value is null. | ||
| /// </summary> | ||
| public string GetCurrentNodeSkipActionContext() |
There was a problem hiding this comment.
GetCurrentNodeSkipActionContext [](start = 22, length = 31)
Update summary to match interface summary. #Closed
There was a problem hiding this comment.
updated, also changed "TreeNode" to "tree node" for consistency.
and updated the comment for private string currentNodeSkipActionContext;
In reply to: 505915013 [](ancestors = 505915013)
| /// <summary> | ||
| /// The context value whether the actions in this TreeNodes are skipped. | ||
| /// </summary> | ||
| private string currentNodeSkipActionContext; |
There was a problem hiding this comment.
currentNodeSkipActionContext [](start = 23, length = 28)
Let's use a similar summary as the get method.
/// The string context if the actions in the current TreeNode were skipped, or null if actions were not skipped. #Closed
There was a problem hiding this comment.
| @@ -33,27 +33,58 @@ public class TreeWalkerUnitTests | |||
|
|
|||
| private Guid sessionId; | |||
| private IForgeDictionary forgeState = new ForgeDictionary(new Dictionary<string, object>(), Guid.Empty, Guid.Empty); | |||
| private ITreeWalkerCallbacks callbacks; | |||
| private ITreeWalkerCallbacksV2 callbacksV2; // 20201012, switch to V2 callback. | |||
There was a problem hiding this comment.
// 20201012, switch to V2 callback. [](start = 51, length = 48)
Please remove this comment. I don't think timestamped comments when code was changed are helpful. #Closed
There was a problem hiding this comment.
| @@ -153,6 +184,19 @@ public void TestTreeWalkerSession_VisitNode_Success() | |||
| Assert.AreEqual(expected, actualNextTreeNodeKey, "Expected VisitNode(Container) to return Tardigrade."); | |||
| } | |||
|
|
|||
| [TestMethod] | |||
| public void TestTreeWalkerSession_VisitNode_Success_WithV1Callbacks() | |||
There was a problem hiding this comment.
TestTreeWalkerSession_VisitNode_Success_WithV1Callbacks [](start = 20, length = 55)
This test calls VisitNode directly, which avoids use of the ITreeWalkerCallbacks completely. Instead, copy the TestTreeWalkerSession_WalkTree_Success test. #Closed
There was a problem hiding this comment.
fixed. and updated a few test methods to cover V1 with File/ForgeTree, and V2 with ForgeTree
In reply to: 505916607 [](ancestors = 505916607)
There was a problem hiding this comment.
The "VisitNode" tests are at the top, followed by "WalkTree" tests.
Please remove this VisitNode test and add a new test just under TestTreeWalkerSession_WalkTree_Success.
In reply to: 505946715 [](ancestors = 505946715,505916607)
There was a problem hiding this comment.
Just noticed the name ordering, I renamed the two cases and moved them into *WalkTree" session
In reply to: 506627465 [](ancestors = 506627465,505946715,505916607)
| currentNodeSkipActionContext:null | ||
| ); | ||
|
|
||
| // Follow the previous Callbacks with ITreeWalkerCallbacks. |
There was a problem hiding this comment.
// Follow the previous Callbacks with ITreeWalkerCallbacks. [](start = 24, length = 59)
Remove this duplicate and misplaced comment. #Closed
There was a problem hiding this comment.
| } | ||
| else | ||
| { | ||
| // Always call this callback, whether or not visit node was successful. |
There was a problem hiding this comment.
// Always call this callback, whether or not visit node was successful. [](start = 28, length = 71)
Let's move this comment so it is valid for V2 or V1. Perhaps above the if statement.
// Always call AfterVisitNode, even if VisitNode threw exception. #Closed
There was a problem hiding this comment.
| this.Parameters.SessionId, | ||
| current, | ||
| await this.EvaluateDynamicProperty(this.Schema.Tree[current].Properties, null), | ||
| this.Parameters.UserContext, | ||
| this.Parameters.TreeName, | ||
| this.Parameters.RootSessionId, | ||
| this.walkTreeCts.Token).ConfigureAwait(false); | ||
| } | ||
|
|
||
| this.currentNodeSkipActionContext = null; // Clear the context of SkipAction and get ready for the next tree node. |
There was a problem hiding this comment.
// Clear the context of SkipAction and get ready for the next tree node. [](start = 68, length = 72)
Let's keep comments on their own lines. #Closed
There was a problem hiding this comment.
// Clear the context of SkipAction before visiting next node because it is only valid locally for this current tree node.
In reply to: 505919008 [](ancestors = 505919008)
There was a problem hiding this comment.
| /// </summary> | ||
| /// <param name="jsonSchema">The json string content.</param> | ||
| /// <param name="treeName">The tree name.</param> | ||
| public void TestInitializeWithFile_WithV1Callbacks(string jsonSchema, string treeName = null) |
There was a problem hiding this comment.
TestInitializeWithFile [](start = 20, length = 22)
This isn't initializing with filePath. We can call it TestInitializeWithJsonSchema_WithV1Callbacks #Closed
There was a problem hiding this comment.
|
|
||
| this.session = new TreeWalkerSession(this.parameters); | ||
| } | ||
|
|
||
| public void TestInitializeWithForgeTree(ForgeTree forgeTree, string treeName = null) |
There was a problem hiding this comment.
public void TestInitializeWithForgeTree [](start = 8, length = 39)
Let's move this initializer just under TestInitialize(..) so they are next to each other. The V1 initializers can follow. #Closed
There was a problem hiding this comment.
moved the V1 initializers to follow the regular initializers.
In reply to: 506625855 [](ancestors = 506625855)
| } | ||
|
|
||
| /// <summary> | ||
| /// Helper function to test V1 ITreeWalkerCallbacks on the contructor accepting FileSchema. |
There was a problem hiding this comment.
FileSchema [](start = 88, length = 10)
jsonSchema #Closed
There was a problem hiding this comment.
| /// The ITreeWalkerCallbacks interface defines the callback Tasks that are awaited while walking the tree. | ||
| /// Prefers future callers to use the new ITreeWalkerCallbacksV2, and keep this ITreeWalkerCallbacks for backward compatibility. |
There was a problem hiding this comment.
, and keep this ITreeWalkerCallbacks for backward compatibility [](start = 68, length = 63)
Remove this comment. #Closed
There was a problem hiding this comment.
| namespace Microsoft.Forge.TreeWalker | ||
| { | ||
| using System; | ||
| using System.Threading; |
There was a problem hiding this comment.
using System.Threading; [](start = 4, length = 23)
I don't think this using statement is required either, please remove if possible. #Closed
There was a problem hiding this comment.
CancellationToken is from "System.Threading". Need to keep as it is.
In reply to: 506629130 [](ancestors = 506629130)
| /// <param name="treeName">The name of the ForgeTree in the JsonSchema.</param> | ||
| /// <param name="rootSessionId">The unique identifier for the root/parent tree walking session.</param> | ||
| /// <param name="currentNodeSkipActionContext"> | ||
| /// The string context if the actions in this tree node are skipped. It is null when no skip. When it is not empty, proceed with ChildSelector. |
There was a problem hiding this comment.
The string context if the actions in this tree node are skipped. It is null when no skip. When it is not empty, proceed with ChildSelector. [](start = 12, length = 139)
The string context if the actions in the current tree node should be skipped, or null if actions should not be skipped. #Closed
There was a problem hiding this comment.
| /// <param name="userContext">Optional, the user context for this tree node.</param> | ||
| /// <param name="token">The cancellation token.</param> | ||
| /// <param name="treeName">The name of the ForgeTree in the JsonSchema.</param> | ||
| /// <param name="rootSessionId">The unique identifier for the root/parent tree walking session.</param> |
There was a problem hiding this comment.
Let's re-use the same param summary from ITreeWalkerCallbacks. #Closed
There was a problem hiding this comment.
| /// <summary> | ||
| /// The callback Task that is awaited before visiting each node. | ||
| /// </summary> | ||
| /// <param name="treeNodeContext">The TreeNode context holding relevant information for this Action.</param> |
There was a problem hiding this comment.
The TreeNode context holding relevant information for this Action. [](start = 42, length = 66)
The tree node context holding relevant information about this tree node and session. #Closed
There was a problem hiding this comment.
updated. applied the same to AfterVisitNode() below.
In reply to: 506633634 [](ancestors = 506633634)
| using System.Threading; | ||
|
|
||
| /// <summary> | ||
| /// The TreeNodeContext class object is passed to Callbacks.BeforeVisitNode and Callbacks.AfterVisitNode. |
There was a problem hiding this comment.
The TreeNodeContext class object is passed to Callbacks.BeforeVisitNode and Callbacks.AfterVisitNode. [](start = 8, length = 101)
The TreeNodeContext class holds relevant information about the tree node and session.
This object gets passed to CallbacksV2.BeforeVisitNode and CallbacksV2.AfterVisitNode. #Closed
There was a problem hiding this comment.
| /// <summary> | ||
| /// The unique identifier for the root/parent tree walking session. | ||
| /// </summary> | ||
| public Guid RootSessionId { get; private set; } |
There was a problem hiding this comment.
Let's use the summaries from ITreeWalkerCallbacks. #Closed
There was a problem hiding this comment.
| @@ -180,8 +192,48 @@ public class TreeWalkerParameters | |||
| this.SessionId = sessionId; | |||
| this.JsonSchema = jsonSchema; | |||
| this.ForgeState = forgeState; | |||
| this.Token = token; | |||
| this.Callbacks = callbacks; | |||
There was a problem hiding this comment.
Flip the order here. Callbacks before token to line up with parameters. #Closed
There was a problem hiding this comment.
| this.Token = token; | ||
| this.Callbacks = callbacks; |
There was a problem hiding this comment.
Flip the order here. Callbacks before token to line up with parameters. #Closed
There was a problem hiding this comment.
…backs