-
Notifications
You must be signed in to change notification settings - Fork 233
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: Add emitter event types and fix mypy #550
Conversation
Signed-off-by: Alex Bozarth <[email protected]>
While working #415 I started addressing a lot of mypy issues as they came and I found that the majority of my changes had become mypy fixes. As such I decided to push what I have at this point since my event types work done so far is currently backwards compatible and could be merged as is. My next step is to start updating the actual |
Signed-off-by: Alex Bozarth <[email protected]>
Signed-off-by: Alex Bozarth <[email protected]>
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.
- I would consider extracting event classes to
events.py
per submodule if possible. - Message[Any] could be replaced with a type alias
AnyMessage
- Changing event names is a breaking change -> should be documented (mentioned somewhere).
Followup tasks from review:
|
BREAKING CHANGE: rename "newToken" event to "new_token" Signed-off-by: Alex Bozarth <[email protected]>
I've pushed the updates addressing the review items I did not mention in my above comment to handle in a followup |
Signed-off-by: Alex Bozarth <[email protected]>
While waiting for review I took a stab at "extracting event classes to I can create a types.py file for backend, tools, and workflows if that's what we want though @Tomas2D |
Signed-off-by: Alex Bozarth <[email protected]>
I was also able to address users extending emitter events dict rather than overwriting it, so no more followup PR work from review remaining |
Signed-off-by: Alex Bozarth <[email protected]>
Signed-off-by: Alex Bozarth <[email protected]>
So after some work leveraging PyCharm's "Refactor > Move File" feature I created the |
FYI I've enabled auto-merge with the requested merge commit message (BREAKING CHANGE) |
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.
Well done. I like it, just the one event name and feel free to merge.
Signed-off-by: Alex Bozarth <[email protected]>
Which issue(s) does this pull-request address?
Ref #415
Ref #433
Description
This include both the first part of typing the emitter event emits and addressing mypy:
Event Type work includes
events.py
file for each submoduleEvent Types work TODOs
MyPy updates
beeai_framework/
except forbeeai_framework/adapters
andbeeai_framework/memory
AnyTool
,AnyAgent
andAnyMessage
types.py
for thebackend
,tools
, andworkflows
submodules and move the relevant classes into those filesChecklist
General
/ TypeScript
Python
for Python changes,TypeScript
for TypeScript changesCode quality checks
poe lint
orpoe lint --fix
/ TypeScriptyarn lint
oryarn lint:fix
poe format
orpoe format --fix
/ TypeScript:yarn format
oryarn format:fix
poe type-check
Testing
poe test --type unit
/ TypeScriptyarn test:unit
poe test --type e2e
/ TypeScript:yarn test:e2e
poe test --type integration
Documentation
poe docs --type build