refactor(giskard-checks)!: remove Scenario.from_sequence#2325
refactor(giskard-checks)!: remove Scenario.from_sequence#2325
Conversation
Use Scenario(name=...).extend(...) instead. Update README and tests. Made-with: Cursor
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 streamlines the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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.gh.umua.top/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request refactors the Scenario class by removing the from_sequence class method. All existing usages of Scenario.from_sequence in documentation, core logic, and test files have been updated to use the Scenario(...).extend(...) fluent API. This change consolidates scenario initialization parameters like name and trace_type into the Scenario constructor, simplifying the extend method's role to solely adding components. Corresponding test class and function names were also updated to reflect this API change.
| scenario = Scenario(name="programmatic_scenario").extend( | ||
| Interact(inputs="Hello", outputs=lambda inputs: "Hi"), | ||
| Equals(expected_value="Hi", key="trace.last.outputs"), | ||
| name="programmatic_scenario", | ||
| ) | ||
|
|
There was a problem hiding this comment.
This change replaces Scenario.from_sequence with Scenario(name=...).extend(...), which aligns with the refactoring goal. This change improves the readability and maintainability of the code by using a more fluent and intuitive API for creating scenarios.
| class TestScenarioExtendAndSerialization: | ||
| """Test Scenario.extend() and step-based serialization.""" |
There was a problem hiding this comment.
Renaming the test class to TestScenarioExtendAndSerialization accurately reflects the focus on testing the extend method and serialization, aligning with the refactoring changes.
| async def test_extend_with_only_checks(self): | ||
| """extend handles scenario starting with checks only.""" |
There was a problem hiding this comment.
Renaming the test function to test_extend_with_only_checks accurately reflects the test's purpose, which is to verify that the extend method correctly handles scenarios starting with checks only.
| scenario = Scenario[Any, Any, MessageTraces]( | ||
| name="test_single_message", | ||
| trace_type=MessageTraces, | ||
| ).extend( |
There was a problem hiding this comment.
The change from using Scenario[Any, Any, MessageTraces].from_sequence to Scenario[Any, Any, MessageTraces](...).extend(...) aligns with the refactoring goal of removing from_sequence and using extend for scenario creation. Initializing name and trace_type directly in the Scenario constructor improves code clarity.
| expected_value="Hello, I want to apply for a job.", | ||
| key="trace.interactions[-1].metadata['tests.integration.test_stateless.mock_apply_tool']['call_args'].args[1]", |
There was a problem hiding this comment.
Removing the name and trace_type parameters from the extend call is consistent with the refactoring, as these parameters are now handled in the Scenario constructor. This change simplifies the extend call and makes the code more readable.
2 participants
Use Scenario(name=...).extend(...) instead. Update README and tests.