Skip to content
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: allow multiple handlers #66

Merged
merged 19 commits into from
Apr 11, 2024
Merged

Conversation

fubuloubu
Copy link
Member

@fubuloubu fubuloubu commented Apr 8, 2024

What I did

Refactor to dynamic registration of tasks, saving tasks (and their handlers) in a separate collection that can be accessed via an enum type to specify their class

Relates to #57, as it migrates to a dynamic registration method via TaskIQ

Also should allow #55 (comment) to use dynamic registration to resolve the bug with multiple handlers with different filters not fully registering

How I did it

New TaskIQ dynamic broker registration API

How to verify it

Everything should work as it did before, don't think this is breaking but wanted to call attention to commits to make sure they were reviewed fully for breaking behaviors

Checklist

  • Passes all linting checks (pre-commit and CI jobs)
    - [ ] New test cases have been added and are passing
    - [ ] Documentation has been updated
  • PR title follows Conventional Commit standard (will be automatically included in the changelog)

@fubuloubu fubuloubu requested review from mikeshultz, antazoey, NotPeopling2day and johnson2427 and removed request for antazoey April 8, 2024 23:34
@fubuloubu fubuloubu mentioned this pull request Apr 8, 2024
4 tasks
silverback/middlewares.py Outdated Show resolved Hide resolved
silverback/middlewares.py Outdated Show resolved Hide resolved
silverback/middlewares.py Outdated Show resolved Hide resolved
silverback/application.py Show resolved Hide resolved
silverback/middlewares.py Outdated Show resolved Hide resolved
@mikeshultz mikeshultz mentioned this pull request Apr 9, 2024
4 tasks
silverback/application.py Outdated Show resolved Hide resolved
johnson2427

This comment was marked as off-topic.

silverback/application.py Outdated Show resolved Hide resolved
silverback/application.py Outdated Show resolved Hide resolved
silverback/middlewares.py Outdated Show resolved Hide resolved
@fubuloubu

This comment was marked as off-topic.

@johnson2427

This comment was marked as off-topic.

@fubuloubu

This comment was marked as off-topic.

silverback/exceptions.py Show resolved Hide resolved
silverback/middlewares.py Outdated Show resolved Hide resolved
silverback/middlewares.py Outdated Show resolved Hide resolved
silverback/types.py Outdated Show resolved Hide resolved
silverback/types.py Show resolved Hide resolved
antazoey
antazoey previously approved these changes Apr 10, 2024
Copy link
Member

@antazoey antazoey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good on my end, see mypy, it also is confused by StrEnum like me I think

silverback/application.py Show resolved Hide resolved
silverback/middlewares.py Show resolved Hide resolved
silverback/application.py Outdated Show resolved Hide resolved
Copy link
Contributor

@johnson2427 johnson2427 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@fubuloubu fubuloubu merged commit 87672ac into main Apr 11, 2024
26 checks passed
@fubuloubu fubuloubu deleted the refactor/allow-multiple-handlers branch April 11, 2024 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants