Refactor so builder context only stores Blocks#432
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the builder context mechanism to only store Block objects (including Link objects) instead of any Refable object. The builder context push is now handled in the init metaclass. This simplification removes unnecessary complexity while maintaining the same functionality.
Key changes:
- Builder context stack now only contains
BaseBlockobjects, simplifying the stack management - The context push/pop mechanism is moved into the metaclass
__init__hooks for bothBlockandLinkclasses ConstraintExpr.parentis renamed toConstraintExpr._contextto better distinguish it from binding parent references
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| edg/core/Builder.py | Refactored Builder to only store BaseBlock in the stack; deprecated get_curr_context() in favor of get_enclosing_block() |
| edg/core/Core.py | Removed _lexical_parent and manual builder push from LibraryElement.__init__; ElementMeta now only sets _block_context |
| edg/core/HierarchyBlock.py | Modified BlockMeta to handle builder context push/pop in the metaclass wrapper and unconditionally pass through args/kwargs |
| edg/core/Link.py | Added LinkMeta metaclass to handle builder context push/pop for Link objects |
| edg/core/ConstraintExpr.py | Renamed parent field to _context for clarity |
| edg/core/Blocks.py | Updated references to use _block_context and improved error messages |
| edg/core/MultipackBlock.py | Updated to use renamed _context field |
| edg/core/Ports.py | Removed redundant _block_context field declaration |
| edg/parts/JlcFet.py | Wrapped lambda instantiation in tuple to ensure lazy evaluation |
| edg/jlcparts/JlcPartsFet.py | Wrapped lambda instantiation in tuple to ensure lazy evaluation |
| edg/jlcparts/JlcPartsDiode.py | Wrapped lambda instantiation in tuple to ensure lazy evaluation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| new_kwargs[arg_name] = kwargs[arg_name] | ||
|
|
||
| orig_init(self, *new_args, **new_kwargs) | ||
| finally: |
There was a problem hiding this comment.
Corrected spelling of 'unconditioally' to 'unconditionally'.
edg/core/ConstraintExpr.py
Outdated
| """Returns a clone of this object with the specified binding. This object must be unbound.""" | ||
| assert not self._is_bound() | ||
| assert builder.get_curr_context() is self.parent, f"can't clone in original context {self.parent} to different new context {builder.get_curr_context()}" | ||
| assert builder.get_enclosing_block() is self._context, f"can't clone in original context {self._context} to different new context {builder.get_curr_context()}" |
There was a problem hiding this comment.
The error message uses the deprecated get_curr_context() method instead of get_enclosing_block(). For consistency with the assertion condition and to use the non-deprecated API, this should be changed to builder.get_enclosing_block().
| assert builder.get_enclosing_block() is self._context, f"can't clone in original context {self._context} to different new context {builder.get_curr_context()}" | |
| assert builder.get_enclosing_block() is self._context, f"can't clone in original context {self._context} to different new context {builder.get_enclosing_block()}" |
Having context be any Refable was overkill and didn't do anything except for one clone check, This changes builder context to only store Blocks (including Links), and has the context push happen in the init metaclass.
Other changes:
__init__hook to unconditionally pass through args and kwargs, instead of looking for *args, **kwargs.