feat: add support for context var index #111
Conversation
WalkthroughThe latest changes enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant JsonTemplateEngine
participant JsonTemplateParser
participant JsonTemplateTranslator
User->>JsonTemplateEngine: call reverseTranslate(expr, options)
JsonTemplateEngine->>JsonTemplateParser: parse expr
JsonTemplateParser-->>JsonTemplateEngine: return parsed result
JsonTemplateEngine->>JsonTemplateTranslator: translate parsed result
JsonTemplateTranslator-->>JsonTemplateEngine: return translated result
JsonTemplateEngine-->>User: return final string
Poem
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
src/engine.ts (1)
Line range hint
19-19: Address the use of 'Function' as a type.The static analysis has flagged the use of 'Function' as a type, which is a potential source of bugs due to its broad acceptance of any function-like values.
- private readonly fn: Function; + private readonly fn: (data: unknown, bindings: Record<string, unknown>) => unknown;This change will specify the function shape explicitly, reducing potential bugs and improving code safety.
Also applies to: 21-21, 25-25, 34-34
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- src/engine.ts (1 hunks)
- src/parser.ts (2 hunks)
- src/reverse_translator.test.ts (3 hunks)
- src/translator.ts (3 hunks)
- src/types.ts (1 hunks)
- src/utils/converter.ts (6 hunks)
- test/scenarios/context_variables/data.ts (1 hunks)
- test/scenarios/mappings/context_vars_mapping.json (1 hunks)
- test/scenarios/mappings/data.ts (3 hunks)
- test/types.ts (1 hunks)
- test/utils/scenario.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- test/scenarios/mappings/context_vars_mapping.json
Additional context used
Biome
src/engine.ts
[error] 19-19: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 21-21: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 25-25: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 34-34: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
src/translator.ts
[error] 907-907: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
src/parser.ts
[error] 1519-1519: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 1525-1525: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 1532-1532: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 1578-1578: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 562-562: Do not add then to an object.
(lint/suspicious/noThenProperty)
Additional comments not posted (21)
test/types.ts (3)
7-7: Field addition to Scenario type:mappings.This field allows specifying mappings directly in the scenario, enhancing flexibility for test configurations.
7-7: Field addition to Scenario type:mappingsPath.This field allows specifying a path to the mappings, which can be loaded dynamically. This addition is useful for scenarios where mappings are too large or need to be shared across tests.
Line range hint
11-11: EnsuregetTemplatePathhandles new fields correctly.The function now correctly prioritizes
templatePath,mappingsPath, andtemplate. This change supports more flexible scenario definitions.test/utils/scenario.ts (3)
12-12: Adjust default path type based on mappings presence.This modification to
getDefaultPathTypemakes the path type dynamic based on the presence of mappings, which is a sensible enhancement for handling different scenario configurations.
21-25: Enhanced scenario initialization with dynamic mappings loading.Loading mappings from a specified path if available and converting them to a template if necessary ensures that scenarios are flexible and can be dynamically configured.
29-33: Fallback to default template handling.The logic to ensure a template is always defined, either through direct assignment or by loading from a file, is robust and prevents scenarios where no template would be set.
src/reverse_translator.test.ts (1)
Line range hint
16-46: Comprehensive test for reverse translation with complex mappings.This test efficiently checks the new functionality of handling arrays of mappings in
reverseTranslate. The mappings and expected results are well-defined, ensuring the method's correctness.test/scenarios/context_variables/data.ts (1)
4-20: New test scenario for context variable handling.This scenario is well-constructed to test the handling of context variables in the last part of a template. The inputs and expected outputs are correctly aligned with the test's purpose.
src/engine.ts (1)
123-129: EnhancedreverseTranslateto handle multiple input types.The method now correctly handles both
ExpressionandFlatMappingPaths[], which increases the flexibility of the engine. The implementation ensures that array inputs are parsed into mappings before translation, which is a crucial enhancement.src/types.ts (1)
287-287: Approved change toFlatMappingASTtype.The change from
PathExpressiontoExpressionforinputExprincreases flexibility in handling different types of expressions. Ensure all usages ofinputExpracross the codebase are compatible with this broader type.test/scenarios/mappings/data.ts (3)
118-136: New test scenario for context variable mappings.This test scenario verifies the handling of context variables within mappings. Ensure that the logic for context variable indexing and transformation is correctly implemented in the engine to support these scenarios.
172-186: New test scenario for array mappings in the last part.This scenario tests the handling of array mappings when they are used in the last part of a mapping path. It's crucial to verify that these mappings correctly resolve to the expected output structures without errors.
491-521: New test scenarios for array to scalar transformations.These scenarios test the transformation of array elements to scalar values. They are important for validating the correctness of array handling logic in scenarios where array elements are expected to transform into scalar values directly.
src/utils/converter.ts (4)
15-15: EnsurePathTypeimport is utilized appropriately.The import of
PathTypeis crucial for handling path expressions in various utility functions. Verify thatPathTypeis used correctly wherever path expressions are handled or evaluated.
43-59: Refactored handling of array index filters.This change introduces more robust handling of array index filters, especially when they are part of the last segment in a path. Ensure that the logic correctly handles cases where the index is out of bounds or when the target is not an array.
Line range hint
73-135: Enhanced handling of 'all' filters in path expressions.These changes introduce more comprehensive handling of 'all' filters in path expressions, allowing for better transformation and integration of data based on complex filtering criteria. Verify that the new handling logic does not introduce any regressions in filter processing.
162-192: Improved processing logic for 'all' filters.This segment significantly improves the processing logic for 'all' filters, ensuring that they are handled efficiently and correctly throughout various parts of the mapping process. It's important to test these changes thoroughly to ensure that they handle edge cases effectively.
src/translator.ts (2)
313-325: Added method to check for 'all' filter expressions.This addition provides a utility method to check if an expression is an 'all' filter, which is crucial for optimizing filter processing logic. Ensure that this method is used consistently across the codebase to improve the efficiency of filter evaluations.
794-800: Refined handling of object filter expressions.This change improves the handling of object filter expressions, particularly those not involving 'all' filters. It's important to ensure that this logic correctly evaluates the filter conditions and applies them appropriately without introducing side effects.
src/parser.ts (2)
1553-1567: Refactor path expression updatesThe method
updatePathExprhas been updated to handletoArrayoptions more explicitly and to convert function calls within path expressions. However, there are several uses ofthisin a static context, which can lead to confusion and potential errors.- JsonTemplateParser.setPathTypeIfNotJSON(newPathExpr, PathType.RICH); + JsonTemplateParser.setPathTypeIfNotJSON(newPathExpr, PathType.RICH); // Correct usage of class name in static contextLikely invalid or redundant comment.
1446-1448: Updated method name and logic for path conversionRenamed from
pathContainsVariablestoshouldPathBeConvertedAsBlockand updated to check forindexoriteminoptionsofPATH_OPTIONStype parts. Ensure that all calls to this method are updated to reflect the new name and logic.Verification successful
Updated method name and logic for path conversion
Renamed from
pathContainsVariablestoshouldPathBeConvertedAsBlockand updated to check forindexoriteminoptionsofPATH_OPTIONStype parts. Ensure that all calls to this method are updated to reflect the new name and logic. The verification confirms that the method name has been updated correctly in the codebase.
- Verified occurrences:
src/parser.ts: private static shouldPathBeConvertedAsBlock(parts: Expression[]): boolean {src/parser.ts: const shouldConvertAsBlock = JsonTemplateParser.shouldPathBeConvertedAsBlock(newPathExpr.parts);Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `shouldPathBeConvertedAsBlock` match the new name and logic. # Test: Search for the function usage. Expect: Only occurrences of the new name. rg --type python $'shouldPathBeConvertedAsBlock'Length of output: 78
Script:
#!/bin/bash # Description: Verify all function calls to `shouldPathBeConvertedAsBlock` match the new name and logic. # Test: Search for the function usage. Expect: Only occurrences of the new name. rg --type typescript 'shouldPathBeConvertedAsBlock'Length of output: 86
Script:
#!/bin/bash # Description: Verify all function calls to `shouldPathBeConvertedAsBlock` match the new name and logic. # Find all TypeScript files and search for the function usage within those files. fd --type f --extension ts | xargs rg 'shouldPathBeConvertedAsBlock'Length of output: 275
Coverage report
Show files with reduced coverage 🔻
Test suite run success210 tests passing in 6 suites. Report generated by 🧪jest coverage report action from b2816f0 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #111 +/- ##
===========================================
- Coverage 100.00% 91.55% -8.45%
===========================================
Files 14 18 +4
Lines 4565 4959 +394
Branches 1082 1086 +4
===========================================
- Hits 4565 4540 -25
- Misses 0 410 +410
- Partials 0 9 +9 ☔ View full report in Codecov by Sentry. |
|
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (4)
src/engine.ts (4)
Line range hint
20-20: Avoid usingFunctionas a type.Prefer explicitly defining the function shape. This type accepts any function-like value, which can be a common source of bugs.
- private readonly fn: Function; + private readonly fn: (...args: any[]) => any;
Line range hint
22-22: Avoid usingFunctionas a type.Prefer explicitly defining the function shape. This type accepts any function-like value, which can be a common source of bugs.
- private constructor(fn: Function) { + private constructor(fn: (...args: any[]) => any) {
Line range hint
26-26: Avoid usingFunctionas a type.Prefer explicitly defining the function shape. This type accepts any function-like value, which can be a common source of bugs.
- private static compileAsSync(template: TemplateInput, options?: EngineOptions): Function { + private static compileAsSync(template: TemplateInput, options?: EngineOptions): (...args: any[]) => any {
Line range hint
35-35: Avoid usingFunctionas a type.Prefer explicitly defining the function shape. This type accepts any function-like value, which can be a common source of bugs.
- private static compileAsAsync(templateOrExpr: TemplateInput, options?: EngineOptions): Function { + private static compileAsAsync(templateOrExpr: TemplateInput, options?: EngineOptions): (...args: any[]) => any {
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- src/engine.ts (4 hunks)
- src/utils/converter.ts (9 hunks)
- test/scenarios/mappings/data.ts (4 hunks)
- test/scenarios/mappings/root_context_vars_mapping.json (1 hunks)
Files not summarized due to errors (1)
- test/scenarios/mappings/root_context_vars_mapping.json: Error: Server error. Please try again later.
Additional context used
Biome
src/engine.ts
[error] 20-20: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 22-22: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 26-26: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 35-35: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
GitHub Check: codecov/patch
src/utils/converter.ts
[warning] 57-59: src/utils/converter.ts#L57-L59
Added lines #L57 - L59 were not covered by tests
[warning] 158-159: src/utils/converter.ts#L158-L159
Added lines #L158 - L159 were not covered by tests
[warning] 266-275: src/utils/converter.ts#L266-L275
Added lines #L266 - L275 were not covered by tests
[warning] 418-419: src/utils/converter.ts#L418-L419
Added lines #L418 - L419 were not covered by tests
[warning] 445-445: src/utils/converter.ts#L445
Added line #L445 was not covered by tests
GitHub Check: Coverage annotations (🧪 jest-coverage-report-action)
src/utils/converter.ts
[warning] 418-418: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 419-419: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 445-445: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 417-417: 🌿 Branch is not covered
Warning! Not covered branch
[warning] 417-419: 🌿 Branch is not covered
Warning! Not covered branch
[warning] 441-449: 🌿 Branch is not covered
Warning! Not covered branch
Additional comments not posted (29)
test/scenarios/mappings/root_context_vars_mapping.json (1)
1-10: LGTM!The JSON structure correctly maps values from one path to another.
src/engine.ts (8)
Line range hint
70-83: LGTM!The
validateMappingsmethod correctly validates JSON path mappings.
86-97: LGTM!The
createFlatMappingsASTmethod correctly creates the AST for flat mappings.
99-100: LGTM!The
parseMappingPathsmethod correctly parses mapping paths and converts them to object mappings.
130-136: LGTM!The
reverseTranslatemethod correctly translates expressions or mapping paths back to their original form.
Line range hint
138-140: LGTM!The
convertMappingsToTemplatemethod correctly converts mappings to a template string.
Line range hint
142-144: LGTM!The
evaluateAsSyncmethod correctly evaluates a template synchronously.
Line range hint
146-148: LGTM!The
evaluatemethod correctly evaluates a template asynchronously.
Line range hint
385-406: LGTM!The
validateMappingsForIndexVarmethod correctly validates mappings for index variables.test/scenarios/mappings/data.ts (8)
118-136: LGTM!The test scenario correctly tests context variable mappings.
137-145: LGTM!The test scenario correctly tests invalid mappings.
146-154: LGTM!The test scenario correctly tests invalid mappings.
190-204: LGTM!The test scenario correctly tests index mappings in the last part.
531-540: LGTM!The test scenario correctly tests array mappings in the last part.
541-550: LGTM!The test scenario correctly tests array mappings to a scalar value.
551-560: LGTM!The test scenario correctly tests array mappings to a scalar value.
435-455: LGTM!The test scenario correctly tests context variable mappings.
src/utils/converter.ts (12)
43-59: LGTM!The
processArrayIndexFilterfunction correctly processes array index filters.Tools
GitHub Check: codecov/patch
[warning] 57-59: src/utils/converter.ts#L57-L59
Added lines #L57 - L59 were not covered by tests
73-81: LGTM!The
getPathExpressionForAllFilterfunction correctly gets the path expression for an "all" filter.
87-94: LGTM!The
validateResultOfAllFilterfunction correctly validates the result of an "all" filter.
104-111: LGTM!The
addToArrayToExpressionfunction correctly adds an expression to an array.
113-135: LGTM!The
handleAllFilterIndexFoundfunction correctly handles the case when an "all" filter index is found.
137-143: LGTM!The
findAllFilterIndexfunction correctly finds the index of an "all" filter.
145-160: LGTM!The
handleAllFilterIndexNotFoundfunction correctly handles the case when an "all" filter index is not found.Tools
GitHub Check: codecov/patch
[warning] 158-159: src/utils/converter.ts#L158-L159
Added lines #L158 - L159 were not covered by tests
162-171: LGTM!The
getNextObjectExpressionForAllFilterfunction correctly gets the next object expression for an "all" filter.
176-192: LGTM!The
processAllFilterfunction correctly processes an "all" filter.
Line range hint
256-276: LGTM!The
handleNextPartfunction correctly handles the next part of a mapping.Tools
GitHub Check: codecov/patch
[warning] 266-275: src/utils/converter.ts#L266-L275
Added lines #L266 - L275 were not covered by tests
57-59: Add tests for this code.The lines are not covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 57-59: src/utils/converter.ts#L57-L59
Added lines #L57 - L59 were not covered by tests
158-159: Add tests for this code.The lines are not covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 158-159: src/utils/converter.ts#L158-L159
Added lines #L158 - L159 were not covered by tests
What are the changes introduced in this PR?
What is the related Linear task?
Resolves INT-2240
Please explain the objectives of your changes below
Put down any required details on the broader aspect of your changes. If there are any dependent changes, mandatorily mention them here
Any changes to existing capabilities/behaviour, mention the reason & what are the changes ?
N/A
Any new dependencies introduced with this change?
N/A
Any new generic utility introduced or modified. Please explain the changes.
N/A
Any technical or performance related pointers to consider with the change?
N/A
@coderabbitai review
Developer checklist
My code follows the style guidelines of this project
No breaking changes are being introduced.
All related docs linked with the PR?
All changes manually tested?
Any documentation changes needed with this change?
Is the PR limited to 10 file changes?
Is the PR limited to one linear task?
Are relevant unit and component test-cases added?
Reviewer checklist
Is the type of change in the PR title appropriate as per the changes?
Verified that there are no credentials or confidential data exposed with the changes.
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Tests