-
Notifications
You must be signed in to change notification settings - Fork 97
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix partial recursive aliases and broken Python integ test #1598
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Reviewed everything up to 2cd3821 in 1 minute and 23 seconds
More details
- Looked at
42
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
7
drafted comments based on config settings.
1. engine/language_client_codegen/src/python/generate_types.rs:393
- Draft comment:
The change now outputs types.{name} (and Optional[types.{name}]) without quotes. Ensure that the generated code either uses 'from future import annotations' or that the module structure guarantees that 'types' is available at annotation evaluation time. This change can affect forward references. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
This is a significant change in how type references are generated. Moving from string literals to direct module references does have implications for Python's type checking behavior. The comment identifies a real technical concern about forward references. However, this is code that generates Python code, so the module structure and imports should be handled by the code generator itself, not something the PR author needs to verify.
The comment identifies a real technical issue, but am I sure that this isn't already handled by the code generator? The PR author likely made this change intentionally with the module structure in mind.
Since this is a code generator, module imports and structure should be handled systematically by the generator itself. The PR author changing from string literals to module references was likely an intentional design decision with the module structure already considered.
Delete the comment. While it raises a valid technical point, this is a code generator where module structure should be handled systematically, not something that needs verification on a case-by-case basis.
2. integ-tests/python/baml_client/partial_types.py:345
- Draft comment:
Updated recursive alias dependency to use 'types.JsonValue' instead of a string literal. Confirm that the types module correctly defines 'JsonValue' and that it's imported in this module. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to confirm the definition and import ofJsonValue
from thetypes
module. This falls under asking the author to confirm or double-check something, which is against the rules. The comment does not provide a specific code suggestion or point out a specific issue with the code.
3. integ-tests/python/tests/memory_test.py:5
- Draft comment:
Changed import to use a relative path (from ..baml_client import b). Verify that the module hierarchy and test runner setup allow for relative imports, avoiding ImportError when running tests. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to verify the module hierarchy and test runner setup, which violates the rule against asking the author to confirm or ensure things. It does not provide a specific suggestion or point out a clear issue with the code.
4. engine/language_client_codegen/src/python/generate_types.rs:392
- Draft comment:
Changed recursive alias formatting from a quoted type to an unquoted module reference. Verify that using 'types.{name}' (and Optional[types.{name}]) correctly handles forward references and that the type is defined early enough. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to verify the behavior of a change, which violates the rule against asking for confirmation or verification. It does not provide a specific code suggestion or point out a specific issue that needs to be addressed.
5. integ-tests/python/baml_client/partial_types.py:345
- Draft comment:
Updated RecursiveAliasDependency to reference 'types.JsonValue' directly. Confirm JsonValue is available in the 'types' module to avoid issues with forward references. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the author to confirm the availability ofJsonValue
in thetypes
module, which is a form of asking for confirmation. This violates the rule against asking the PR author to confirm their intention or to double-check things. Therefore, this comment should be removed.
6. integ-tests/python/tests/memory_test.py:5
- Draft comment:
Modified the import to use a relative path. Ensure that the module structure supports 'from ..baml_client import b' in all environments. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
7. engine/language_client_codegen/src/python/generate_types.rs:230
- Draft comment:
There's a minor typographical error in the comment for the add_default_value function. It says 'Short-circuite' but should be 'Short-circuit'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_356WelUfnUOEg9Da
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
#1588
Important
Fix partial recursive types in
generate_types.rs
and correct import path inmemory_test.py
.generate_types.rs
, updateFieldType::RecursiveTypeAlias
to usetypes.{name}
instead of"{name}"
.Optional["{name}"]
toOptional[types.{name}]
forFieldType::RecursiveTypeAlias
.partial_types.py
, changevalue: Optional["JsonValue"]
tovalue: Optional[types.JsonValue]
inRecursiveAliasDependency
.memory_test.py
fromfrom baml_client import b
tofrom ..baml_client import b
.This description was created by
for 2cd3821. It will automatically update as commits are pushed.