Refactor dummy evaluator to use conductor runner API#3693
Refactor dummy evaluator to use conductor runner API#3693yytelliot wants to merge 3 commits intosource-academy:masterfrom
Conversation
Summary of ChangesHello, 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 refactors the dummy evaluator to align with the official conductor runner API. This change ensures the dummy evaluator serves as a more accurate and realistic reference implementation, improving consistency and maintainability by leveraging the established API for conductor interactions rather than a custom, simplified version. 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. 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 refactors the dummy-conductor-evaluator.js to integrate with the new conductor runner API, replacing the previous custom message handling and evaluator setup. This involves removing old channel-related constants and functions, introducing dynamic import for the runner, and adapting the DummyEvaluator to the new API. Feedback includes an issue where the !fileContent check in startEvaluator incorrectly treats empty files as an error, suggesting a more specific check for undefined. Additionally, there's a design concern about the DummyEvaluator constructor having a side effect by assigning to an outer-scoped variable (runnerConductor), which increases coupling and reduces reusability.
| async startEvaluator(entryPoint) { | ||
| await this.evaluateFile(entryPoint, ''); | ||
| const fileContent = await this.conductor.requestFile(entryPoint); | ||
| if (!fileContent) { |
There was a problem hiding this comment.
The check !fileContent will evaluate to true for both undefined (file not found) and an empty string '' (empty file). This means that if the entrypoint file exists but is empty, this code will incorrectly throw an error. An empty file is a valid scenario. You should check specifically for undefined to distinguish between a file that was not found and a file that is empty.
| if (!fileContent) { | |
| if (fileContent === undefined) { |
| class DummyEvaluator { | ||
| constructor(conductor) { | ||
| this.conductor = conductor; | ||
| runnerConductor = conductor; |
There was a problem hiding this comment.
The constructor has a side effect of assigning to runnerConductor, a variable in the outer scope. This couples the DummyEvaluator class to its surrounding implementation, making it less reusable and harder to test. While this works, it's generally best to avoid side effects in constructors. If the runner's initialise API provides a way to get a handle on the conductor instance it creates, it would be cleaner to set up the unhandledrejection listener outside of this class.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| class DummyEvaluator extends BasicEvaluator { | ||
| async evaluateFile(fileName, fileContent) { | ||
| this.conductor.sendOutput('[dummy] output message'); | ||
| this.conductor.sendResult('[dummy] result message'); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Pull Request Test Coverage Report for Build 23564088475Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
4 participants
Description
Made the dummy evaluator use the conductor runner API instead of a dummy conductor class so that it is a more accurate reference.
Type of change
How to test
Checklist