Conversation
Summary of ChangesHello @chrisk314, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience for developers working with event-driven models by refactoring the underlying mechanism for creating event connectors. The changes consolidate the complex, multi-step process of setting up event Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a solid refactoring that significantly simplifies the user experience for creating event connectors. The changes are consistently applied across the codebase, including documentation, examples, and tests, which is great to see. I have a couple of suggestions to further improve the implementation by enhancing type safety and simplifying some of the new logic.
I am having trouble creating individual review comments. Click here to see my feedback.
plugboard/connector/connector_builder.py (22)
The type hint for components should be more specific for better type safety. It should be _t.Iterable[Component] instead of _t.Iterable[_t.Any] to match the signature of EventConnectorSpecBuilder.build. Using a string forward reference "Component" is necessary to avoid a circular import.
To make this work, you'll need to add the following import inside a TYPE_CHECKING block at the top of the file:
if _t.TYPE_CHECKING:
from plugboard.component import Component def build_event_connectors(self, components: _t.Iterable["Component"]) -> list[Connector]:
plugboard/connector/event_connector_spec_builder.py (18-38)
The build method and its helper _build_for_component can be significantly simplified. You can first gather all unique event types from all components into a set, and then create the connector specs in a single dictionary comprehension. This makes the logic more concise and easier to follow.
@staticmethod
def build(components: _t.Iterable[Component]) -> dict[str, ConnectorSpec]:
"""Returns mapping of connector specs for events handled by components."""
all_event_types = {
evt.type
for component in components
for evt in component.io.input_events + component.io.output_events
}
return {
evt_type: EventConnectorSpecBuilder._build_for_event(evt_type)
for evt_type in all_event_types
}
|
@copilot can you resolve the circular import problem in this PR? |
|
@toby-coleman I've opened a new pull request, #208, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
Benchmark comparison for |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
Benchmark comparison for |
Summary
This PR addresses #57 by refactoring the code involved in creating
Connectors forEvents in event-driven models in an attempt to simplify the user experience. Previously multiple lines of code were required involving several library classes putting some burden of understanding low level details on the user. Now these lines can be replaced with a single line of code involving a concreteConnectorclass which will be familiar to users.Changes
EventConnectorBuilderbecomes a utility classEventConnectorSpecBuilderwith its responsibility changed to producing a mapping ofevent.type->ConnectorSpec.ConnectorBuilderexposes a new methodbuild_event_connectors(components: list[Component]) -> list[Connector]which consumes theEventConnectorSpecBuilderto produce the requiredEventConnectors.ConnectorABC has a new class methodbuilderwhich returns aConnectorBuilderfor a givenConnectorconcrete class.RayConnector(previously restriction was applied toRayProcesswhich actually can be used with differentConnectorclasses).