typed Events #264
typed Events #264
Conversation
There was a problem hiding this comment.
The current issue with this is:
MyPy wants a type annotation (even my_event: Event = Event() would be enough).
This means either:
- We find a way to make this work with MyPy without needing the annotation
- All event initializations need to be updated
In my opinion all events should be typed so I would tend to option 2, but of course that would place a slight burden on users using MyPy, too.
|
@NiklasNeugebauer I also like option 2. But we have to see how it turns out once we fixed all linter and type errors: |
|
Hm, you're right, it seems simply typing empty events as empty = Event[[]]()
empty.register(lambda: print('empty'))We could add a global class Event(Generic[P]):
...
@staticmethod
def Empty() -> Event[[]]:
return Event()Otherwise, we could maybe keep the |
|
As far as I can tell, ParamSpec defaults have been introduced in 3.13 https://docs.python.org/3/whatsnew/3.13.html DefaultP = ParamSpec("DefaultP", default=[str, int])
class Foo(Generic[DefaultP]): ...
reveal_type(Foo) # type is type[Foo[DefaultP = [str, int]]]
reveal_type(Foo()) # type is Foo[[str, int]]
reveal_type(Foo[[<
8000
span class="pl-s1">bool, bool]]()) # type is Foo[[bool, bool]]This would be very nice to have |
|
This is the best I could come up with for a warning class Event(Generic[P]):
def __class_getitem__(cls, item):
print('__class_getitem__', cls, item)
setattr(cls, '__getitem_called', True)
return super().__class_getitem__(item)
def __new__(cls, *args, **kwargs):
print('__new__', cls, args, kwargs)
if not hasattr(cls, '__getitem_called'):
log.warning(
'The Event class was instantiated without a type argument. '
'If you wanted to indicate that the event is emitted without any arguments, '
'use Event[[]] instead.'
)
return Event[[]].__new__(cls)
return super().__new__(cls) |
|
I've got some mypy/pylint complaints, so I changed it to def __class_getitem__(cls, _item: Any) -> type[Event[P]]:
setattr(cls, '__getitem_called', True)
return cls
def __new__(cls, *args, **kwargs) -> Event[P]:
if not hasattr(cls, '__getitem_called'):
log.warning(
'The Event class was instantiated without a type argument. '
'If you wanted to indicate that the event is emitted without any arguments, use Event[[]] instead.'
)
return Event[[]].__new__(cls)
return super().__new__(cls, *args, **kwargs)But I wonder if we should raise an exception with a helpful text instead of "fixing" user code. This would also allow to write a pytest more easily. |
|
I'd argue for raising instead of "fixing". While switching to typed events in our code, everything would otherwise assume it's an event without an argument which some new user might have wanted to achieve, but for us wouldn't be the case. |
I did not test this properly before. This does not really work since all Events are of the same class and thus there is only one single I suggest we just don't do it then. |
@falkoschindler @codingpaula I would love you to comment on that, is this PR approved then? |
There was a problem hiding this comment.
@NiklasNeugebauer Oh, I didn't notice that the PR is ready for a final review.
But yes, let's merge this one!
Update to [RoSys 0.23.0](https://github.com/zauberzeug/rosys/releases/tag/v0.23.0) which includes features that were already needed on main. zauberzeug/rosys#264 introduced typed events as part of 0.23.0, some events had to be annotated because of it. Was tested on U4
I have have had a lot of situations where I was not exactly sure which values an event would produce or these values changed but the consumer was not changed correctly with it.
This PR adds a Generic ParamSpec to the
Eventclass which enables users to optinally initialize their events asor equivalently
to let the type checker correctly check the types of the callback in
my_event.register()and the other functions.