Conversation
|
@coderabbitai review |
WalkthroughThe changes introduce a new module in the project structure, enhance the handling of node selections and variants, and modify several functions across multiple files to accommodate new return types and parameters. Key updates include the addition of fields for managing named types, improvements in error handling, and the introduction of new methods for rendering union types in both Rust and TypeScript. Additionally, test cases have been updated to reflect these changes, ensuring comprehensive coverage of the new functionality. Changes
Possibly related PRs
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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: 33
Outside diff range and nitpick comments (18)
tests/metagen/typegraphs/sample.ts (1)
Line range hint
67-73: Approved, but consider improving union type utilization.The uncommented
scalarUnionfunction successfully exposes thescalarUniontype in the API. However, the current implementation always returns a string ("hello"), which doesn't fully utilize the union type capabilities.Consider modifying the function to return either a string or an integer based on some condition. For example:
scalarUnion: deno.func( t.struct({ id: t.string() }), scalarUnion, { code: ({ id }) => id.length % 2 === 0 ? "hello" : 42, }, ),This change would better demonstrate the union type's flexibility and ensure both possible return types are utilized.
tests/metagen/typegraphs/sample/rs/main.rs (2)
107-137: LGTM! New union type queries look good.The implementation of new query operations for
scalar_union,composite_union, andmixed_unionaligns well with the PR objective of introducing union support. The code demonstrates proper use of the new union types and selection structures, providing good examples for different selection scenarios.A few suggestions for improvement:
- Consider adding a brief comment explaining the purpose of the
scalar_unionquery, similar to the comments forcomposite_unionandmixed_union.- The
composite_unionquery is repeated twice with different selections. It might be beneficial to add a comment explaining why this repetition is necessary for testing purposes.
Line range hint
1-176: Overall implementation looks solid, addressing PR objectives.The changes in this file successfully introduce union support for client implementations, aligning well with the PR objectives. The new query operations for
scalar_union,composite_union, andmixed_uniondemonstrate proper use of the new union types and selection structures. The implementation provides good examples of different union selection scenarios, which is valuable for testing purposes.A few points to consider:
- As mentioned in the PR objectives, there might be some edge cases concerning variant identification in client languages. It would be beneficial to add comments or tests specifically addressing these potential edge cases.
- The PR notes that end-user documentation has not yet been updated. Consider adding TODO comments in relevant sections to remind about updating the documentation once the implementation is finalized.
tests/metagen/metagen_test.ts (2)
610-614: LGTM: Improved command execution and output validationThe changes to the command execution logic enhance the test by actually running the command and validating its output. Parsing the output as JSON allows for more precise validation against the expected schema, which is a significant improvement.
Consider adding error handling for the JSON parsing step. For example:
let parsedOutput; try { parsedOutput = JSON.parse(res); } catch (error) { throw new Error(`Failed to parse command output as JSON: ${error.message}`); } expected.parse(parsedOutput);This will provide more informative error messages if the command output is not valid JSON.
Line range hint
1-614: Overall assessment: Changes improve union type support and testingThe modifications in this file enhance the testing of union type support across different client implementations (Rust, TypeScript, and Python). The changes align well with the PR objectives and improve the overall test coverage.
Key improvements:
- Updated schema definition for
compositeUnion2.- New test case for the Rust client implementation.
- Enhanced command execution and output validation for all client tests.
To ensure the robustness of these changes:
- Verify that all union type scenarios are adequately covered in the tests.
- Ensure that the build and test processes for all client implementations (especially the new Rust client) are properly integrated into the CI/CD pipeline.
- Consider adding more detailed error handling and logging to facilitate debugging in case of test failures.
These changes provide a solid foundation for union type support in the metagen implementations. Great work on improving the test coverage and consistency across different client languages!
tests/metagen/typegraphs/sample/ts/main.ts (1)
85-113: Consider extracting the repeated ID into a constant for maintainabilityThe same ID string
"94be5420-8c4a-4e67-b4f4-e1b2b54832a2"is used multiple times in your query definitions. Extracting it into a constant can improve maintainability and make future updates easier.Apply this diff to extract the ID into a constant:
Add the following line above line 85:
+const commonId = "94be5420-8c4a-4e67-b4f4-e1b2b54832a2";Then update the usages in lines 87, 90, 97, and 104:
tests/metagen/typegraphs/sample/py/main.py (2)
110-139: Enhancement: Define the repeatedidas a constantThe
idvalue"94be5420-8c4a-4e67-b4f4-e1b2b54832a2"is hardcoded multiple times in the new queries. Defining it as a constant improves readability and maintainability, especially if the value needs to change in the future.Apply this diff to define a constant and use it throughout:
+TEST_ID = "94be5420-8c4a-4e67-b4f4-e1b2b54832a2" ... res5 = gql_client.query( { "scalarUnion": qg.scalar_union( { - "id": "94be5420-8c4a-4e67-b4f4-e1b2b54832a2", + "id": TEST_ID, } ), "compositeUnion1": qg.composite_union( { - "id": "94be5420-8c4a-4e67-b4f4-e1b2b54832a2", + "id": TEST_ID, }, {"post": {"_": SelectionFlags(select_all=True)}}, ), "compositeUnion2": qg.composite_union( { - "id": "94be5420-8c4a-4e67-b4f4-e1b2b54832a2", + "id": TEST_ID, }, {"user": {"_": SelectionFlags(select_all=True)}}, ), "mixedUnion": qg.mixed_union( { - "id": "94be5420-8c4a-4e67-b4f4-e1b2b54832a2", + "id": TEST_ID, }, { "post": {"_": SelectionFlags(select_all=True)}, "user": {"_": SelectionFlags(select_all=True)}, }, ), } )
110-139: Add comments to explain each new query for clarityAdding comments before each query helps readers understand the purpose of the query and what it is testing, which is especially useful in test scripts.
Here's how you can add comments:
res5 = gql_client.query( { + # Test scalar union with scalar return type "scalarUnion": qg.scalar_union( { "id": TEST_ID, } ), + # Test composite union with 'post' selection "compositeUnion1": qg.composite_union( { "id": TEST_ID, }, {"post": {"_": SelectionFlags(select_all=True)}}, ), + # Test composite union with 'user' selection "compositeUnion2": qg.composite_union( { "id": TEST_ID, }, {"user": {"_": SelectionFlags(select_all=True)}}, ), + # Test mixed union with both 'post' and 'user' selections "mixedUnion": qg.mixed_union( { "id": TEST_ID, }, { "post": {"_": SelectionFlags(select_all=True)}, "user": {"_": SelectionFlags(select_all=True)}, }, ), } )src/metagen/src/client_ts/selections.rs (1)
68-70: Fix typos in commentsThere are typos in the comments:
- Line 69: "varaint" should be "variant"
- Line 70: "normlalization" should be "normalization"
src/metagen/src/client_py/selections.rs (1)
135-157: Possible optimization: Collect variants without intermediate allocationsIn the section where variants are collected for
render_for_union, consider using iterator methods to avoid intermediate allocations, improving performance.Refactor the code to:
let variants = variants .iter() .filter_map(|&inner| { if !renderer.is_composite(inner) { return None; } let ty_name = renderer.nodes[inner as usize].deref().base().title.clone(); let struct_prop_name = normalize_struct_prop_name(&normalize_type_title(&ty_name)); let selection = selection_for_field( inner, &self.arg_ty_names, renderer, cursor, ).ok()?; Some((struct_prop_name, (ty_name, selection))) }) .collect::<IndexMap<_, _>>();src/metagen/src/client_ts/mod.rs (1)
Line range hint
282-309: Handle potential panics when unwrappingRcandMutexThe use of
Rc::try_unwrap(named_types).unwrap()andinto_inner().unwrap()can lead to a panic ifnamed_typeshas multiple strong references or if theMutexis poisoned. To prevent potential panics, consider handling theErrcases explicitly instead of usingunwrap().Proposed code to handle errors gracefully:
Ok(( memo, Rc::try_unwrap(named_types) .map_err(|_| anyhow::anyhow!("named_types has multiple references"))? .into_inner() .map_err(|e| anyhow::Error::new(e))?, ))This modification uses
map_errto convert potentialErrcases intoanyhow::Error, providing clearer error handling without panicking.src/metagen/src/client_ts/static/mod.ts (3)
Line range hint
1-164: Consider refactoring_selectionToNodeSetfor better maintainabilityThe
_selectionToNodeSetfunction is becoming lengthy and complex, with multiple nested conditions and varying logic for handlingsubNodesandvariants. This complexity can make the code harder to read, understand, and maintain. Consider refactoring the function by extracting parts of the logic into smaller helper functions. This would improve readability and make it easier to manage and test individual components of the code.
107-109: Clarify error message for invalidNodeMetaconfigurationThe error message when both
subNodesandvariantsare defined states "unreachable: union/either NodeMetas can't have subnodes". The term "unreachable" might be misleading if this situation can occur due to incorrect usage. Consider rephrasing the error message to clearly indicate the issue to the developer, such as "Invalid NodeMeta: A NodeMeta cannot have bothsubNodesandvariantsdefined."Apply this diff to improve the error message:
107 throw new Error( -108 "unreachable: union/either NodeMetas can't have subnodes", +108 "Invalid NodeMeta: A NodeMeta cannot have both `subNodes` and `variants` defined.", 109 );
Line range hint
456-473: Enhance content-type check infetchGqlfunctionIn the
fetchGqlfunction, the content-type check uses a strict equality toapplication/json. This may fail if the response includes additional parameters like charset (e.g.,application/json; charset=utf-8). Consider updating the check to use.includes("application/json")for a more robust validation.Apply this diff to improve the content-type check:
- if (res.headers.get("content-type") != "application/json") { + if (!res.headers.get("content-type")?.includes("application/json")) {tests/metagen/typegraphs/sample/ts/client.ts (2)
385-385: Avoid Mutating Variables When PossibleThe variable
gqlTyis reassigned after removing nullability indicators. To maintain immutability and prevent potential side effects, consider using a new variable.Suggested change:
- gqlTy = gqlTy.replace(/[!]+$/, ""); + const baseGqlTy = gqlTy.replace(/[!]+$/, "");And update the usage of
gqlTytobaseGqlTyin the returned string:return `... on ${baseGqlTy} {${ ... }}`;
931-954: Add Documentation for New MethodsThe methods
scalarUnion,compositeUnion, andmixedUnionlack documentation. Adding JSDoc comments will improve readability and help users understand their purpose and usage.Consider adding JSDoc comments to these methods, detailing their parameters and return types.
src/metagen/src/client_rs/static/client.rs (2)
23-24: Improve documentation comment for clarityIn the documentation comment on lines 23-24, consider rephrasing "I.e." to "In other words," or "This means that" for better readability and clarity.
47-47: Correct typo in commentThe comment on line 47 contains a typo: "noe" should be corrected to "node".
Apply this diff to correct the typo:
- // this noe was not selected + // this node was not selected
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (18)
- Cargo.toml (1 hunks)
- src/metagen/src/client_py/mod.rs (4 hunks)
- src/metagen/src/client_py/node_metas.rs (5 hunks)
- src/metagen/src/client_py/selections.rs (3 hunks)
- src/metagen/src/client_py/static/client.py (10 hunks)
- src/metagen/src/client_rs/static/client.rs (6 hunks)
- src/metagen/src/client_ts/mod.rs (4 hunks)
- src/metagen/src/client_ts/node_metas.rs (4 hunks)
- src/metagen/src/client_ts/selections.rs (3 hunks)
- src/metagen/src/client_ts/static/mod.ts (14 hunks)
- tests/metagen/metagen_test.ts (2 hunks)
- tests/metagen/typegraphs/sample.ts (3 hunks)
- tests/metagen/typegraphs/sample/py/client.py (17 hunks)
- tests/metagen/typegraphs/sample/py/main.py (1 hunks)
- tests/metagen/typegraphs/sample/rs/client.rs (12 hunks)
- tests/metagen/typegraphs/sample/rs/main.rs (2 hunks)
- tests/metagen/typegraphs/sample/ts/client.ts (19 hunks)
- tests/metagen/typegraphs/sample/ts/main.ts (1 hunks)
Additional context used
Ruff
src/metagen/src/client_py/static/client.py
383-383: Loop control variable
nodeoverrides iterable it iterates(B020)
tests/metagen/typegraphs/sample/py/client.py
386-386: Loop control variable
nodeoverrides iterable it iterates(B020)
Additional comments not posted (20)
tests/metagen/typegraphs/sample.ts (2)
37-38: LGTM: Union types activated as intended.The uncommented declarations for
compositeUnionandscalarUnionsuccessfully activate these union types in the typegraph. This change aligns with the PR objective of introducing union support for client implementations. The types used in these unions are correctly defined or built-in, which is good.
Line range hint
37-87: Summary: Union support successfully implemented with room for optimization.The changes successfully introduce union support for the
client_xxmetagen implementations as intended. The implementation is correct and functional, aligning well with the PR objectives. However, there are a few points to consider:
- The
mixedUniontype combines composite and scalar types, which might lead to ambiguity in some cases. Exercise caution when designing such union types, as mentioned in the PR description.- The
scalarUnionfunction could be optimized to better utilize the full range of its union type.These changes lay a solid foundation for union support, but keep in mind the potential edge cases mentioned in the PR description, particularly concerning variant identification in client languages. As the implementation progresses, it may be beneficial to update the end-user documentation to reflect these new capabilities and potential considerations when using union types.
Cargo.toml (1)
8-8: LGTM. Verify integration of the new module.The addition of "src/metagen/src/client_rs/static" as a workspace member is consistent with the PR objectives for introducing union support in client metagen implementations. This change expands the project structure to include new Rust code for client-side static generation.
To ensure proper integration, please run the following verification script:
This script will help verify the existence of the new module, its basic structure, and its integration with other parts of the project.
tests/metagen/typegraphs/sample/rs/main.rs (1)
166-171: JSON output updated correctly.The JSON output has been appropriately updated to include the results of the new union queries (
scalarUnion,compositeUnion1,compositeUnion2, andmixedUnion). This change ensures that the results of all new query operations are properly captured and displayed in the output.tests/metagen/metagen_test.ts (2)
571-571: LGTM: Schema change forcompositeUnion2The change from
zod.undefined()tozod.object({})forcompositeUnion2aligns with the PR objective of introducing union support. This modification allowscompositeUnion2to accept an empty object instead of undefined.Please ensure that this change doesn't break any existing test cases and that it correctly handles empty objects in the union type validation.
578-583: LGTM: New test case forclient_rsThe addition of the
client_rstest case is a positive change that ensures the Rust client implementation is properly tested. It uses the sameexpectedSchemaas other client implementations, maintaining consistency across different languages.Please ensure that:
- The Rust client's build process is properly integrated with this test.
- The
cargo runcommand executes successfully in the CI/CD pipeline.- The test covers all the union type scenarios introduced in this PR.
tests/metagen/typegraphs/sample/ts/main.ts (1)
85-115: LGTM!The new queries using union types are correctly implemented and effectively demonstrate the new functionality introduced in this PR.
tests/metagen/typegraphs/sample/py/main.py (1)
110-139: Verification: Confirm that union types are correctly usedEnsure that the union types
scalar_union,composite_union, andmixed_unionare being used correctly according to the GraphQL schema. Verify that the selections provided are valid for the possible types within each union.Run the following script to list possible types for each union and ensure the selections match:
This script fetches the schema and lists the union types along with their possible types. Ensure that the selections in your queries correspond to these types.
src/metagen/src/client_py/selections.rs (1)
128-161: Ensure consistent handling ofTypeNode::EitherandTypeNode::UnionvariantsThe logic for processing
TypeNode::EitherandTypeNode::Unionfilters out non-composite variants. However, if there are scalar variants, they are currently being ignored silently. This might lead to incomplete type definitions in the generated code.Run the following script to check if there are any scalar variants in
EitherorUniontypes that might need handling:src/metagen/src/client_ts/node_metas.rs (1)
92-120:render_for_unionmethod implementation looks goodThe new
render_for_unionmethod correctly constructs the union type rendering logic. The formatting and iteration over variants are well-implemented.src/metagen/src/client_py/node_metas.rs (2)
61-64: Ensure correct string interpolation forreturn_nodeAt line 61, the code writes
return_node = NodeDescs.{return_node}(). Verify that{return_node}is correctly interpolated with the actual return node name in the generated code. Ensure that the placeholder{return_node}is properly formatted and will be replaced as expected during code generation.
96-121: Verify correctness of therender_for_unionmethodThe newly implemented
render_for_unionmethod appears to generate the Python code for union types. Ensure that the generated code has the correct syntax and indentation, and that all variants are properly included in thevariantsdictionary.src/metagen/src/client_ts/mod.rs (1)
127-127: Confirm correct handling of new return type fromrender_node_metasThe function
render_node_metasnow returns a tuple(NameMemo, HashSet<u32>). The updated code correctly captures bothnode_metasandnamed_types. Ensure that all usages ofrender_node_metasare updated accordingly throughout the codebase.Run the following script to verify all calls to
render_node_metashandle the new return type:Verification successful
All
render_node_metasusages correctly handle the new return typeVerified that all calls to
render_node_metasproperly unpack the returned tuple(node_metas, named_types).Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all instances of `render_node_metas` and check for correct tuple unpacking. # Expected: All calls should unpack the tuple returned by `render_node_metas`. rg 'let\s+\(\s*[^)]+\s*\)\s*=\s*render_node_metas' --type rustLength of output: 445
src/metagen/src/client_ts/static/mod.ts (2)
216-217: Consistency inNodeMetatype usage across the codebaseWith the addition of the
variantsproperty to theNodeMetatype, ensure that all usages ofNodeMetathroughout the codebase are updated accordingly. This includes places whereNodeMetais constructed or destructured, to prevent potential runtime errors or type mismatches.Run the following script to find all usages of
NodeMetaand check for necessary updates:#!/bin/bash # Description: Find all usages of 'NodeMeta' and verify updates. # Search for 'NodeMeta' type references rg --type ts 'NodeMeta' src/metagen/src/ # Optionally, check for destructuring assignments that may need updates rg --type ts '{.*} =.*NodeMeta' src/metagen/src/
367-391: Ensure proper handling ofsubNodesinconvertQueryNodeGqlThe
convertQueryNodeGqlfunction now handlessubNodesthat can be either an array or a record of variants. Verify that whensubNodesis aRecord<string, SelectNode[]>, the generated GraphQL query correctly represents union types using inline fragments (... on Type { ... }).Run the following script to check that union types are correctly transformed into GraphQL queries:
tests/metagen/typegraphs/sample/ts/client.ts (2)
731-735: Verify Argument Type ConsistencyThe
argumentTypesforRootCompositeArgsFnspecifyidas"RootScalarNoArgsFnOutput", which appears to be an output type. Ensure that the argument type is correct and represents the expected input type.Please check if
RootScalarNoArgsFnOutputis the appropriate type for theidargument or if it should be something likeUserIdStringUuid.
836-847: Ensure Accuracy of Union Type DefinitionsThe union types
RootCompositeUnionFnOutput,RootMixedUnionFnOutput, andRootScalarUnionFnOutputcombine various types. Confirm that all included types are correct and necessary for the intended functionality.Review the union type definitions to ensure they represent all valid output types without including unintended ones.
tests/metagen/typegraphs/sample/py/client.py (1)
14-14: Simplified Parameter Type withNodeMetaFnThe parameter
metasin theselection_to_nodesfunction is updated to useNodeMetaFn, which is an alias fortyping.Callable[[], "NodeMeta"]. This change simplifies the type signature and enhances code readability.src/metagen/src/client_rs/static/client.rs (2)
122-123: Re-evaluate the use ofunreachable!macroIn lines 122-123, the code uses
unreachable!("union/either node metas can't have sub_nodes"). Ensure that this case is truly impossible. If there's a possibility that bothvariantsandsub_nodesareSome, consider handling it gracefully instead of panicking.
Line range hint
1057-1069: Check macro expansion for potential issuesIn lines 1057-1069, the macro
impl_union_selection_traitsis being used. Ensure that the macro expansion correctly generates code without compilation errors, especially regarding the handling ofCompositeSelection::Union.Run the following script to examine the macro expansion:
#!/bin/bash # Description: Expand macros in the Rust code to check for correctness. # Test: Use `cargo expand` to see the expanded code for `impl_union_selection_traits`. cargo expand --lib src/metagen/src/client_rs/static/client.rs | rg 'impl_union_selection_traits' -A 50Review the expanded code to verify that it aligns with the intended implementation.
2 participants
client_xxmetagen implementations.There are still some edge cases especially around variant identification in the client languages. I tried many things but our hands are tied by serde. Basically, users will have to be careful when designing their union types to avoid ambiguity cases. Hopefully, 674 will help there.
Migration notes
...
Summary by CodeRabbit
New Features
variantsproperty to theNodeMetatype for improved selection handling.Bug Fixes
Tests