-
Notifications
You must be signed in to change notification settings - Fork 1
fix(oocana): extract _wrap_output_with_warning to reduce duplication #454
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
base: main
Are you sure you want to change the base?
Conversation
The output() and outputs() methods had duplicate try-except blocks for handling ValueError and IOError exceptions. This refactoring: - Extracts _wrap_output_with_warning() method for common error handling - Returns (value, success) tuple to simplify control flow - Reduces code duplication while maintaining identical behavior - Simplifies target handling logic in output()
演进概览在 变更
预估代码审查工作量🎯 2 (简单) | ⏱️ ~10 分钟 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR extracts duplicate try/except error handling patterns from the output() and outputs() methods into a reusable helper method _wrap_output_with_warning() to reduce code duplication.
Changes:
- Added new helper method
_wrap_output_with_warning()that wraps output values with error handling and returns a success tuple - Refactored
output()method to use the new helper method instead of inline try/except blocks - Refactored
outputs()method to use the new helper method instead of inline try/except blocks - Minor code cleanup including whitespace fixes and type annotation improvements
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """ | ||
| return os.getenv("OOMOL_TOKEN", "") | ||
|
|
||
| def _wrap_output_with_warning(self, key: str, value: Any) -> tuple[Any, bool]: |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency with other helper methods in the Context class (such as __wrap_output_value, __is_basic_type, __store_ref, __dataframe, __matplotlib), this method should use double underscores (__wrap_output_with_warning) rather than a single underscore to indicate it's a private method.
| def _wrap_output_with_warning(self, key: str, value: Any) -> tuple[Any, bool]: | ||
| """ | ||
| Wrap output value with error handling. | ||
|
|
||
| Args: | ||
| key: The output handle key | ||
| value: The value to wrap | ||
|
|
||
| Returns: | ||
| A tuple of (wrapped_value, success). If success is False, wrapped_value is None. | ||
| """ | ||
| try: | ||
| return self.__wrap_output_value(key, value), True | ||
| except (ValueError, IOError) as e: | ||
| self.send_warning(f"{e}") | ||
| return None, False |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR description mentions that three methods (output, outputs, and finish) had identical try/except patterns that should be refactored. However, the finish() method at lines 514-523 still contains the duplicate try/except pattern and was not updated to use the new _wrap_output_with_warning() helper method. This method should also be refactored for consistency.
Summary
_wrap_output_with_warning()method to handle output wrapping with error handlingoutput(),outputs(), andfinish()methodsProblem
Three methods had identical try/except patterns:
Solution
Extracted to a single helper method that returns
(wrapped_value, success)tuple.Test Plan