cleanup validateHandlePathExists#26469
Conversation
| const pathParts = handle.split("/").slice(1); | ||
| const currentPath = pathParts[0]; | ||
| let found = false; | ||
| for (const [key, summaryObject] of Object.entries(summaryTree.tree)) { |
There was a problem hiding this comment.
Looping over all the keys of an object to find currentPath is a very silly way to find a given key on an object. Now I just use the [] operator.
| assert( | ||
| summaryObject.type === SummaryType.Tree || summaryObject.type === SummaryType.Handle, | ||
| `Handle path ${currentPath} should be for a subtree or a handle`, | ||
| currentObject.type === SummaryType.Handle, | ||
| `Handle path ${handle} should be for a subtree or a handle`, | ||
| ); | ||
| if (summaryObject.type === SummaryType.Tree) { | ||
| validateHandlePathExists(`/${pathParts.slice(1).join("/")}`, summaryObject); | ||
| } | ||
| } |
There was a problem hiding this comment.
the way this if and assert were structured needlessly checked the Tree case twice, and also silently ignored the case when the path resolved to a handle and there was still more path to check. I have refactored this and added an assert.fail in the unhandled case to fail tests instead of silently not validating paths inside handles.
There was a problem hiding this comment.
Pull request overview
This PR refactors the validateHandlePathExists helper function in a test file to simplify the logic for validating handle paths in summary trees. The refactoring replaces a recursive implementation with an iterative approach that uses a loop and explicit type checking.
Changes:
- Replaced recursive implementation with iterative loop-based approach
- Converted JSDoc comment to single-line comments
- Added explicit validation that handle paths start with "/" and contain at least one segment
- Simplified the tree traversal logic with clearer type guards
packages/dds/tree/src/test/feature-libraries/forest-summary/forestSummarizer.spec.ts
Outdated
Show resolved
Hide resolved
packages/dds/tree/src/test/feature-libraries/forest-summary/forestSummarizer.spec.ts
Outdated
Show resolved
Hide resolved
| ); | ||
| } | ||
| } | ||
| assert(found, `Handle path ${currentPath} not found in summary tree`); |
There was a problem hiding this comment.
These asserts are needlessly hard to interpret as they only contain a single element of the path. I have rewritten the function to be iterative instead of recursive so the whole path is in scope for better error, and we avoid splitting the string again at every level.
| * The handle path is split by "/" into pathParts where the first element should exist in the root | ||
| * of the summary tree, the second element in the first element's subtree, and so on. | ||
| */ | ||
| const pathParts = handle.split("/").slice(1); |
There was a problem hiding this comment.
if there was part of the path before the first slash, this silently ignored it, which seems wrong, so I assert instead.
1 participant
Description
cleanup validateHandlePathExists
Reviewer Guidance
The review process is outlined on this wiki page.