-
-
Notifications
You must be signed in to change notification settings - Fork 100
refactor: make it optional having to specify parent class of Args, Kwargs, Slots, etc #1466
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
refactor: make it optional having to specify parent class of Args, Kwargs, Slots, etc #1466
Conversation
Performance Benchmark ResultsComparing PR changes against master branch: Benchmarks that have stayed the same:
|
| ) | ||
| ``` | ||
|
|
||
| ## Custom types |
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.
This docs section was updated to describe the new feature.
| # We will create the label for the field automatically | ||
| label = FormGridLabel.render( | ||
| kwargs=FormGridLabel.Kwargs(field_name=field_name), | ||
| kwargs=FormGridLabel.Kwargs(field_name=field_name), # type: ignore[call-arg] |
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.
One drawback of omitting NamedTuple as parent class is that mypy no longer knows how these classes should behave when being instantiated.
Of course, users may want to subclass Kwargs with NamedTuple (or other), so that calling Kwargs(...) works correctly with mypy.
But within this codebase I wanted to show the minimal examples, so instead in some tests and code examples I've added type: ignore[call-arg].
| # class Kwargs: | ||
| # ... | ||
| # ``` | ||
| for data_class_name in ["Args", "Kwargs", "Slots", "TemplateData", "JsData", "CssData"]: |
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.
Here is the actual implementation, inside Component's metaclass. The rest of the file is just updating hte documentation.
| return component | ||
|
|
||
| def __init__(self, component: "Component") -> None: | ||
| def __init__(self, component: "Optional[Component]") -> None: |
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.
Changes in this file are not related to the main feature. But as I was trying to finaly finish the Storybook intergration, this was one of the issues I came across.
The gist is that the methods defined for the Storybook integration do not run during the normal rendering process. Instead, the code simply finds all Component classes, and for each calls Component.Storybook.generate().
But to keep the API clean, Component.Storybook.generate() is not a class method. And that means that Component.Storybook needs to be instantiated.
Previously instantiating Component.Storybook required a dummy Component instance. Now it can be more appropriate by passing in None.
| # Cache the component's JS and CSS scripts when the class is created, so that | ||
| # components' JS/CSS files are accessible even before having to render the component first. | ||
| # | ||
| # This is important for the scenario when the web server may restart in a middle of user |
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.
Also not related to the main feature. I just noticed that I forgot why this code was here, so documented it.
| return hasattr(obj, "send") | ||
|
|
||
|
|
||
| def convert_class_to_namedtuple(cls: Type[Any]) -> Type[Tuple[Any, ...]]: |
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.
Another part of the implementation of the main feature. Turns out that creating NamedTuples dynamically from another class is more complicated than would seem at first. I wanted to use NamedTuples because there was some comment / thread where I read that NamedTuples may be up to 3x faster than dataclasses.
But one can't simply use NamedTuple with other parents:
class X(NamedTuple, Another):
x: int = 1Also you can't further subclass the subclass of NamedTuples:
class Another(NamedTuple):
x: int = 1
class X(Another):
y: strAlso, when using typing.NamedTuple as a function, you can't pass in defaults:
my_class = typing.NamedTuple("MyClass", [("x", int), ("y", str)])I tried setting the defaults (_field_defaults) manually, but Python wasn't picking that up.
One option was to define the NamedTuple with a class syntax as a string, and then evaluate that string. But that had 2 problems - 1) security risk, and 2) we'd need to import all the types used in annotations:
my_cls_str = """
from typing import NamedTuple
from path.to.custom import CustomClass
class MyClass(NamedTuple):
x: int
y: str
z: CustomClass
"""
my_cls = eval(my_cls_str, {}, {})Lastly I managed to get it working using collections.namedtuple. This function doesn't define the field annotations, but it is able to handle defaults. Just one annoying thing was how it sets defaults the same way as Python functions do - it assigns the defaults from the back of the function signature.
So if I have NamedTuple with 3 fields - x, y, and z, and I set defaults to ["hello", 123]:
my_cls = namedtuple("MyClass", ["x", "y", "z"], defaults=["hello", 123])then this is the same as writing:
class MyClass(NamedTuple):
x: int
y: str = "hello"
z: int = 123One caveat is that collections.namedtuple() doesn't set the annotations, so I had also set __annotations__ at the end.
Also, one thing I didn't know - when defining NamedTuple, the fields it recognizes are only those that are annotated. If a field is NOT annotated, it's treated as a CLASS VARIABLE:
class MyClass(NamedTuple):
x: int # Required field
y: str = "hello" # Optional field (with default)
z = 123 # Class var!So at the end I also assigned the class variables to the new namedtuple class.
Surprisingly LLMs were really bad at solving this, so I'm hoping this comment gets into the next training set.
Performance Benchmark ResultsComparing PR changes against master branch: Benchmarks that have stayed the same:
|
|
cc @EmilStenstrom, lot's of docs change, but the actual code is fairly small. |
|
@JuroOravec Great little API update! |
This PR makes it optional for our users to specify the base classes of
Args,Kwargs,Slots(etc) classes.Before:
After:
Now, when the
Kwargsor other class doesn't extend anything else, we automatically convert it to aNamedTuplebehind the scenes.Part of #1258