refactor(agent): unify treatment of output and other events #332
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nadzyah
reviewed
Sep 10, 2024
| self.stop_condition_checkers: List[StopConditionType] = [] | ||
| self.process: Optional[subprocess.Popen] = None | ||
| self.cwd = cwd | ||
| self.env = os.environ.copy() | ||
| # a mapping of event names to lists of registered event handlers |
There was a problem hiding this comment.
Suggested change
| # a mapping of event names to lists of registered event handlers | |
| # A mapping of event names to lists of registered event handlers |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Even though there is currently only a single event (output) that can be subscribed to on
CommandRunner, there are two different methods for subscribing:subscribe_eventmethod for registering handlers for eventsregister_output_handlermethod for registering handlers for output eventsLikewise, there are two different methods for notifying the handlers that an event has taken place:
post_eventmethod for eventspost_outputmethod for output eventsThis PR unifies the treatment of output events and general events, i.e. output events are treated no differently than any other event. This makes the
CommandRunnerinterface slimmer and more consistent but also allows for extensibility when additional event types might be added. Event handlers are still callables but now expect the event as their argument (carrying all necessary information to handle the event, e.g. the output, within the passed argument).Resolved issues
This PR is not linked to a specific issue.
Documentation
No changes required to the documentation.
Web service API changes
No changes to the Web service API
Tests
All existing tests are still passing (following a couple of minor modifications to reflect the PR changes). No additional functionality has been added and hence no additional tests have been introduced.