-
Notifications
You must be signed in to change notification settings - Fork 12
Refactor so builder context only stores Blocks #432
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
Conversation
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.
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: |
Copilot
AI
Nov 12, 2025
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.
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()}" |
Copilot
AI
Nov 12, 2025
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.
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.