feat(python): Support Reference Aliases #4938
Conversation
WalkthroughThe pull request introduces modifications to the 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: 0
🧹 Outside diff range and nitpick comments (6)
generators/pythonv2/codegen/src/ast/Reference.ts (1)
49-53: LGTM with a minor suggestion:writemethod update andgetAliasmethod additionThe changes to the
writemethod correctly handle the inclusion of the alias in the output when present. The newgetAliasmethod provides appropriate access to thealiasprivate member.Consider using the newly added
getAliasmethod in thewritemethod for consistency:- if (this.alias) { + if (this.getAlias()) { writer.write(" as "); - writer.write(this.alias); + writer.write(this.getAlias()!); }This change would make the code more consistent and easier to maintain in the future.
Also applies to: 72-74
generators/pythonv2/codegen/src/ast/__test__/Reference.test.ts (1)
53-60: LGTM! Consider adding a test with module path.The new test case for handling classes with aliases is well-structured and aligns with the PR objectives. It effectively verifies the
toString()method's behavior when a reference includes an alias.To further enhance test coverage, consider adding another test case that includes both a module path and an alias. This would ensure that the
toString()method correctly handles more complex scenarios. For example:it("handles class with module path and alias", () => { const reference = python.reference({ name: "AliasClass", modulePath: ["module", "submodule"], alias: "Alias" }); expect(reference.toString()).toBe("AliasClass as Alias"); });This additional test would verify that the presence of a module path doesn't affect the alias representation.
generators/pythonv2/codegen/src/ast/PythonFile.ts (1)
96-98: Consider handling edge cases for aliases.While the implementation is correct, consider adding a check to ensure the alias is not an empty string. This would prevent generating invalid Python syntax in edge cases.
Here's a suggested improvement:
-if (reference.getAlias()) { +const alias = reference.getAlias(); +if (alias && alias.trim() !== '') { - writer.write(` as ${reference.getAlias()}`); + writer.write(` as ${alias.trim()}`); }This change ensures that only non-empty aliases are included in the import statement.
generators/pythonv2/codegen/src/ast/__test__/PythonFile.test.ts (3)
115-136: LGTM! Consider adding an explicit assertion.The test case effectively demonstrates the use of an absolute import with an alias. It follows good testing practices and uses snapshot testing appropriately.
Consider adding an explicit assertion to verify the presence of the aliased import statement before the snapshot comparison. This could provide more specific feedback if the test fails. For example:
const output = writer.toString(); expect(output).toContain("from external_module.submodule import ExternalClass as AliasedClass"); expect(output).toMatchSnapshot();
138-159: LGTM! Consider adjusting the relative import path.The test case effectively demonstrates the use of a relative import with an alias. It follows good testing practices and uses snapshot testing appropriately.
The
modulePathin the reference suggests a relative import, but it's using an absolute path. Consider adjusting it to use a relative path for consistency with the test case description. For example:const relativeRef = python.reference({ modulePath: ["..", "sibling_dir"], name: "SiblingClass", alias: "AliasedSibling" });Also, consider adding an explicit assertion to verify the presence of the aliased import statement before the snapshot comparison, similar to the suggestion for the previous test case.
114-159: Great addition of test cases for aliased imports!The new test cases effectively cover both absolute and relative imports with aliases, aligning well with the PR objectives. They enhance the test coverage for the
PythonFileclass and demonstrate good testing practices.Consider adding more edge cases to further strengthen the test suite, such as:
- Multiple aliased imports in a single file
- Aliased imports with conflicting names
- Nested aliased imports
These additional test cases would help ensure robustness of the new feature across various scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
generators/pythonv2/codegen/src/ast/__test__/__snapshots__/PythonFile.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
- generators/pythonv2/codegen/src/ast/PythonFile.ts (1 hunks)
- generators/pythonv2/codegen/src/ast/Reference.ts (3 hunks)
- generators/pythonv2/codegen/src/ast/test/PythonFile.test.ts (1 hunks)
- generators/pythonv2/codegen/src/ast/test/Reference.test.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (5)
generators/pythonv2/codegen/src/ast/Reference.ts (3)
17-18: LGTM: Addition ofaliasproperty toArgsinterfaceThe new
aliasproperty is correctly implemented as an optional string, which aligns with the PR objective of supporting reference aliases. The comment provides a clear description of its purpose.
26-26: LGTM: Addition ofaliasprivate memberThe new
aliasprivate member is correctly typed asstring | undefined, which is consistent with the optionalaliasproperty in theArgsinterface.
28-33: LGTM: Constructor update andaliasassignmentThe constructor signature has been correctly updated to include the
aliasparameter, and the assignment of thealiasparameter to the private member is properly implemented.generators/pythonv2/codegen/src/ast/PythonFile.ts (2)
95-98: LGTM! Correct implementation of import aliases.The changes correctly implement the support for import aliases in Python. The conditional block is well-placed, and the alias is written in the correct format. This enhancement aligns with the PR objective and improves the flexibility of import statements in generated Python code.
Line range hint
1-111: Overall assessment: Well-implemented feature with minor suggestion.The changes in this file successfully implement the support for reference aliases in Python imports. The implementation is correct and aligns well with the PR objectives. The suggested minor improvement for handling edge cases would further enhance the robustness of the code, but the current implementation is already functional and valuable.
| exports[`PythonFile > Add a class with a relative import and alias 1`] = ` | ||
| "from ...test_module.test.sibling_dir import SiblingClass | ||
|
|
||
| class TestClassWithRelativeAlias(SiblingClass as AliasedSibling): |
There was a problem hiding this comment.
[blocking] this looks incorrect I don't think python supports inline as aliasing within the base class inheritance
There was a problem hiding this comment.
Ah true good catch. Will fix in a follow-up.
Add support for reference aliases
Adds support for specifying the alias used for reference imports.
Summary by CodeRabbit
New Features
Tests
toStringmethod to include references with aliases.