Disable default constructor in specialized Quantity types
#465
Conversation
github-actions
bot
added
part:data-pipeline
shsms
force-pushed
the
quantity-improvements
branch
from
892ba52 to
d7272fa
Compare
llucax
left a comment
llucax
left a comment
There was a problem hiding this comment.
Nice! Only 1 (serious) comment.
| def disable_default_constructor(cls: type[_T]) -> type[_T]: | ||
| """Disable the default constructor for a class. | ||
| Check that the class does not already have a default constructor, and add one that | ||
| raises a TypeError if called. | ||
| Args: | ||
| cls: The class to disable the default constructor for. | ||
| Returns: | ||
| The class with the default constructor disabled. | ||
| Raises: | ||
| TypeError: If the class already has a default constructor. | ||
| """ | ||
| if "__init__" in cls.__dict__: | ||
| raise TypeError(f"No default constructor allowed for {cls.__name__}") | ||
|
|
||
| def __init__(self: Any, *args: Any, **kwargs: Any) -> None: | ||
| raise TypeError( | ||
| f"Use of default constructor NOT allowed for {cls.__name__}. " | ||
| f"Use one of the `{cls.__name__}.from_*()` methods instead." | ||
| ) | ||
|
|
||
| cls.__init__ = __init__ # type: ignore[method-assign] | ||
| return cls |
There was a problem hiding this comment.
I'm a bit scared about patching classes via a decorator now, after the headaches this brought with the @actor decorator. I guess for this case where you are only adding a __init__ that probably won't confuse mypy it should be OK, but I fear in the future someone might think it is a good idea to add some other common method here and things go south again.
Other approaches that might be more mypy friendly are just defining a base type _NoDefaultConstructible and inherit from it (but that won't catch if the error if someone adds an __init__ to it, although it would be weird to inherit from _NoDefaultConstructible and add an __init__. But another alternative would be metaclasses, I'm not sure how mypy feels about those, but I think is the most widely method used to manipulate classes, so my guess is that probably better than decorators.
For example: https://stackoverflow.com/questions/8212053/private-constructor-in-python/64682734#64682734
This also adds the _create() method that could actually be useful to abstract sub-classes from dealing with __new__() directly.
There was a problem hiding this comment.
I've replaced it with a meta class. Didn't add a private constructor, because it has to be typed with Any, whereas with __new__, we get proper typing.
shsms
force-pushed
the
quantity-improvements
branch
3 times, most recently
from
a40db61 to
d4dc2cb
Compare
shsms
force-pushed
the
quantity-improvements
branch
from
d4dc2cb to
d566feb
Compare
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
This is because support for default constructors for derived types is going away in a subsequent commit. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
This commit also updates the FormulaBuilders to take custom constructors as arguments. FormulaEngines were previously taking the type of the quantity, with which, it could use only the default constructors to create objects. Because support for using default constructors is going away, FormulaEngines and FormulaBuilders need to know the exact constructor to use for creating output quantities. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
This is because the default constructors for specialized quantity types will stop working in a subsequent commit. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
This is so that there are no remaining dependencies on the default quantity constructor, before it is disabled for specialized quantites. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Specialized quantities can only be instantiated through custom constructors after this. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
shsms
force-pushed
the
quantity-improvements
branch
from
d566feb to
7ac01de
Compare
added for all my recently merged PRs. |
2 participants
No description provided.