Conversation
|
Going to pull Eric in since his Python knowledge far exceeds mine! |
eneumann
left a comment
There was a problem hiding this comment.
Nice work on this refactor @harshi922!
A few of these edge cases look like the kind that static typing catches early (especially around optional TypedDict keys and changed return contracts). It may be worth adding a stronger type-checking extension to your vscode so you can spot these early on.
For example, my user settings (global settings) for python uses Pyright via Pylance in basic mode. Then most projects i work with also use mypy.
|
|
||
|
|
||
| def render_spec(spec: SpecificationDto) -> Tuple[str, str]: | ||
| def render_spec(spec: SpecificationDto) -> Tuple[str, str, str]: |
There was a problem hiding this comment.
With this return-contract change, please make sure all callers can handle it (e.g. correct unpacking for all three values) This one in rendered_spec.py will break -> func_str, type_defs = render_spec(spec)
| f"'{function_name}', function unavailable: \" + str(e))" | ||
| ) | ||
|
|
||
| indented = '\n '.join(runtime_code.split('\n')) |
There was a problem hiding this comment.
It's possible this can generate invalid Python when there’s no runtime code left after type extraction (e.g. some function that only defines types). In that case we still emit a try: block with an empty body, which causes an error. Let's add a fallback like pass, when runtime_code is empty just to be safe.
| module_scope_names: set[str] = set(class_names) | ||
|
|
||
| # Get all module-level assigned symbol names for reference | ||
| module_level_symbols: set[str] = { |
There was a problem hiding this comment.
I think there’s one runtime edge case here: class definitions can be moved to module scope before their imported dependencies are available. For example, with import pydantic + class A(pydantic.BaseModel), the class may be emitted outside the try, while import pydantic stays inside it. Can we treat imported symbols as module-scope dependencies too, so classes and their required imports stay aligned?
| prefix = """logger = logging.getLogger("poly") | ||
| try: | ||
| # X = Literal[...], X = Dict[str, Any], X = list[Foo], X = Union[...] | ||
| if isinstance(node, ast.Subscript): |
There was a problem hiding this comment.
I think this check might be a bit too broad and could move some regular code into the type section, which could unintentionally change runtime behavior.
#5933
Okay this is a big change but this makes it possible to not define types in CFX 3X. Tested against actual GHA CFX - all ok! Will work in server func logic similarly once this gets changes suggested and resolved!