fix: stop get_response #323
Conversation
WalkthroughThe pull request introduces modifications to three key files in the OVOS workshop: Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🔇 Additional comments (3)
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ovos_workshop/skills/game_skill.py (2)
121-132: Good implementation with room for improvementThe implementation provides a clean way to programmatically stop games. Consider these enhancements:
- Add type hints for the return value
- Document the return value in the docstring
- Consider handling the paused state explicitly in the logic flow
- def stop_game(self): + def stop_game(self) -> bool: """to be called by skills if they want to stop game programatically + + Returns: + bool: True if game was stopped, False if it wasn't playing + """ if self.is_playing: + # Handle pause state first + if self.is_paused: + self._paused.clear() - self._paused.clear() self.gui.release() self.log.debug("changing OCP state: PlayerState.STOPPED ") self.bus.emit(Message("ovos.common_play.player.state", {"state": PlayerState.STOPPED})) self._playing.clear() self.on_stop_game() return True return False
Line range hint
283-289: Consider improving error handling granularityThe current try-except block encompasses multiple operations that could fail independently. Consider handling autosave and game abandonment separately for better error recovery.
def handle_deactivate(self, message: Message): try: if self.is_paused: self.log.info("Game is paused, keeping it active") self.activate() # keep the game in active skills list so it can still converse elif self.is_playing: - self._autosave() - self.log.info("Game abandoned due to inactivity") - self.on_abandon_game() - self.stop_game() + try: + self._autosave() + except Exception as e: + self.log.error(f"Autosave failed during deactivation: {e}") + + try: + self.log.info("Game abandoned due to inactivity") + self.on_abandon_game() + except Exception as e: + self.log.error(f"Game abandonment handling failed: {e}") + + try: + self.stop_game() + except Exception as e: + self.log.error(f"Game stop failed: {e}") except Exception as e: self.log.exception(f"Error during game deactivation: {e}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ovos_workshop/skills/game_skill.py(3 hunks)ovos_workshop/skills/ovos.py(3 hunks)
🔇 Additional comments (5)
ovos_workshop/skills/ovos.py (2)
1351-1356: Enhanced session stopping with proper cleanup.The changes improve session stopping by:
- Combining results from both
stop_sessionandstopmethods- Adding cleanup of ongoing
get_responseoperationsThis ensures a more thorough cleanup when stopping sessions.
2154-2154: Added retry control for selection prompts.The addition of the
num_retriesparameter provides better control over user interaction retries in theask_selectionmethod. This is a good enhancement for handling cases where users might need multiple attempts to make a valid selection.Also applies to: 2192-2192
ovos_workshop/skills/game_skill.py (3)
6-6: LGTM: Import addition is appropriateThe addition of
PlayerStatefromovos_utils.ocpis necessary for the new stopping mechanism and follows the existing import pattern.
134-136: LGTM: Clean callback implementationThe stop method is well-implemented as a callback that delegates to stop_game. The docstring clearly indicates its intended usage.
287-287: LGTM: Consistent use of stop_gameThe change to use stop_game ensures consistent cleanup behavior across all stopping scenarios.
SWIM fix it
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #323 +/- ##
==========================================
- Coverage 49.27% 49.08% -0.19%
==========================================
Files 37 37
Lines 4426 4441 +15
==========================================
- Hits 2181 2180 -1
- Misses 2245 2261 +16 ☔ View full report in Codecov by Sentry. |
companion to OpenVoiceOS/ovos-core#643
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes