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: Use type[Event] instead of str to filter events #6480

Merged
merged 4 commits into from
Jan 27, 2025

Conversation

malhotra5
Copy link
Contributor

@malhotra5 malhotra5 commented Jan 27, 2025

End-user friendly description of the problem this fixes or functionality that this introduces

  • Include this change in the Release Notes. If checked, you must provide an end-user friendly description for your change below

Give a summary of what the PR does, explaining any non-trivial design decisions

The get_matching_events function in EventStream was comparing event types using string names. This change makes it use actual class types instead, which provides:

Type safety - type checker ensures only Event subclasses are passed
Refactoring safety - class renames will update all references
Better IDE support with code completion
No risk of typos in string names

The changes we made:

  1. Changed the type of event_type parameter from str | None to type[Event] | None in both functions
  2. Changed the comparison from event.class.name == event_type to isinstance(event, event_type)
  3. Serialize event_type from string to Event for FASTApi endpoint

Link of any specific issues this addresses


To run this PR locally, use the following command:

docker run -it --rm   -p 3000:3000   -v /var/run/docker.sock:/var/run/docker.sock   --add-host host.docker.internal:host-gateway   -e SANDBOX_RUNTIME_CONTAINER_IMAGE=docker.all-hands.dev/all-hands-ai/runtime:ccec685-nikolaik   --name openhands-app-ccec685   docker.all-hands.dev/all-hands-ai/openhands:ccec685

@malhotra5 malhotra5 marked this pull request as draft January 27, 2025 18:04
@malhotra5 malhotra5 marked this pull request as ready for review January 27, 2025 18:16
@malhotra5 malhotra5 enabled auto-merge (squash) January 27, 2025 18:35
@@ -361,7 +361,7 @@ def _should_filter_event(
def get_matching_events(
self,
query: str | None = None,
event_type: str | None = None,
event_type: type[Event] | None = None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you're so inclined, this change actually allows you to tighten up the return value annotation as well:

T = TypeVar('T', Event)
def get_matching_events(
    ...,
    event_type: type[T] | None = None,
    ...
) -> list[T]:
    ...

Or, using the condensed syntax in Python 3.12 (from this PEP):

def get_matching_events[T: Event](
    ...,
    event_type: type[T] | None = None,
    ...
) -> list[T]:
    ...

@malhotra5 malhotra5 disabled auto-merge January 27, 2025 18:37
@malhotra5 malhotra5 merged commit 6045349 into main Jan 27, 2025
16 checks passed
@malhotra5 malhotra5 deleted the feature/event-type-class branch January 27, 2025 18:58
zchn pushed a commit to zchn/OpenHands that referenced this pull request Feb 4, 2025
idagelic pushed a commit to idagelic/OpenHands that referenced this pull request Feb 12, 2025
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