Update Component.__init__ to allow for both Python and Flutter#52
Update Component.__init__ to allow for both Python and Flutter#52
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Component.__init__ method to support two calling conventions: the traditional positional children arguments and a new keyword-only children parameter that accepts an Iterable. This change appears to facilitate interoperability between Python and Flutter implementations.
Changes:
- Added
overloadimport to support multiple type signatures - Introduced two overload signatures: one for positional children (
*children) and one for keyword-only children (children: Iterable) - Modified the
__init__implementation to handle both calling patterns
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/pydom/component.py
Outdated
| def __init__(self, /, *, children: Iterable["ChildType"]) -> None: ... | ||
|
|
||
| def __init__(self, *args, children=None) -> None: | ||
| self.children = children if children is not None else args |
There was a problem hiding this comment.
The implementation has two related issues: (1) Type inconsistency - the children attribute is declared as Tuple["ChildType", ...] (line 8), but when passed as an Iterable via keyword argument, it's assigned directly without conversion to tuple. (2) Codebase convention deviation - the Element class uses tuple(to_iter(children or args)) (element.py:19) to handle children initialization. The Component implementation should follow this same pattern to ensure type safety and consistency across the codebase.
| self.children = children if children is not None else args | |
| if children is not None: | |
| self.children = tuple(children) | |
| else: | |
| self.children = tuple(args) |
src/pydom/component.py
Outdated
| @overload | ||
| def __init__(self, *children: "ChildType") -> None: ... | ||
| @overload | ||
| def __init__(self, /, *, children: Iterable["ChildType"]) -> None: ... |
There was a problem hiding this comment.
The second overload signature has a positional-only marker / followed by keyword-only arguments. The / marker with no positional parameters before it is unusual and may be confusing. Consider removing the / marker as it serves no purpose when there are no positional-only parameters to protect. The signature should simply be def __init__(self, *, children: Iterable["ChildType"]) -> None: ...
| def __init__(self, /, *, children: Iterable["ChildType"]) -> None: ... | |
| def __init__(self, *, children: Iterable["ChildType"]) -> None: ... |
src/pydom/component.py
Outdated
| def __init__(self, *args, children=None) -> None: | ||
| self.children = children if children is not None else args |
There was a problem hiding this comment.
The new functionality to pass children as a keyword argument (e.g., Component(children=[...])) lacks test coverage. Given that the repository has comprehensive test coverage (as seen in test_html_rendering.py and test_json_rendering.py), tests should be added to verify that the new calling convention works correctly for both the keyword-only children parameter and maintains backward compatibility with the positional children arguments.
neriyaco
force-pushed
the
binyamin555-patch-1
branch
from
1306f52 to
c5a3014
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2 participants
No description provided.