modernize #38
modernize #38
Conversation
WalkthroughThe pull request includes updates to several files to enhance the packaging and functionality of the project. The Changes
Poem
Warning Rate limit exceeded@JarbasAl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 2 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
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: 1
🧹 Outside diff range and nitpick comments (5)
requirements.txt (1)
Line range hint
1-4: Consider adding version constraints for all dependenciesThe existing OVOS dependencies lack version constraints while the new
ovos-bus-clientspecifies one. This inconsistency could lead to compatibility issues.Consider adding minimum version constraints for all dependencies:
-ovos-utils -ovos-workshop -ovos-plugin-manager -ovos-audio +ovos-utils>=0.0.0 +ovos-workshop>=0.0.0 +ovos-plugin-manager>=0.0.0 +ovos-audio>=0.0.0Replace
0.0.0with the minimum compatible versions for each package.skill.json (2)
Line range hint
63-63: Fix malformed settings JSON structure.The settings section contains embedded markdown code block markers (```js) within the JSON string, which could cause parsing issues. This should be properly escaped or reformatted.
Consider reformatting the settings section like this:
- "settings": "~/.config/ovos/skills/skill-laugh.openvoiceos/settings.json ~/.config/neon/skills/skill-laugh.openvoiceos/settings.json ```js { \"gender\": \"robot\", // or \"male\" or \"female\" \"haunted\": false, // default true, \"mine is an evil laugh\" \"sounds_dir\": \"/home/neon/venv/lib/python3.10/site-packages/skill_laugh/sounds\", // default on a Neon setup, can be set to anything OVOS/Neon can access \"__mycroft_skill_firstrun\": false } ```" + "settings": { + "config_paths": [ + "~/.config/ovos/skills/skill-laugh.openvoiceos/settings.json", + "~/.config/neon/skills/skill-laugh.openvoiceos/settings.json" + ], + "defaults": { + "gender": "robot", + "haunted": false, + "sounds_dir": "/home/neon/venv/lib/python3.10/site-packages/skill_laugh/sounds", + "__mycroft_skill_firstrun": false + } + }
Line range hint
39-39: Consider adding a proper license identifier.The "license" field is set to "Unknown". Since this is an open-source project, it would be beneficial to specify an appropriate license.
__init__.py (2)
99-105: Consider improving audio-visual synchronization.The current implementation might lead to race conditions between GUI operations and audio playback. Consider coordinating the timing between audio and visual elements for a better user experience.
Suggested improvement:
def laugh(self): """Make the voice assistant laugh.""" sound = random.choice(self.sounds[self.gender]) + sound_duration = self.get_audio_length(sound) # Add helper method self.gui.clear() pic = random.randint(0, 3) - self.gui.show_image(str(pic) + ".jpg") - self.play_audio(sound) - self.gui.clear() + image_dir = join(dirname(__file__), "ui/images") + image_path = join(image_dir, f"laugh_{pic}.jpg") + if not isfile(image_path): + self.log.warning(f"Image not found: {image_path}") + self.play_audio(sound) + return + + self.gui.show_image(image_path) + self.play_audio(sound, wait=True) + # Allow a short delay after audio completes + self.schedule_event(self.gui.clear, + datetime.now() + timedelta(seconds=sound_duration + 0.5))
Line range hint
16-42: Consider enhancing type safety with additional type hints.The property implementations look good, but could benefit from more precise type hints for better code maintainability and IDE support.
Suggested improvements:
@property - def sounds_dir(self) -> str: + def sounds_dir(self) -> Path: """Path to the sounds directory.""" default = join(dirname(__file__), "sounds") if not self.settings.get("sounds_dir"): self.sounds_dir = default - return self.settings.get("sounds_dir", default) + return Path(self.settings.get("sounds_dir", default)) @sounds_dir.setter - def sounds_dir(self, value) -> None: + def sounds_dir(self, value: Union[str, Path]) -> None: """Setter for sounds_dir property.""" - self.settings["sounds_dir"] = value + self.settings["sounds_dir"] = str(value)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (4)
gui/all/0.jpgis excluded by!**/*.jpggui/all/1.jpgis excluded by!**/*.jpggui/all/2.jpgis excluded by!**/*.jpggui/all/3.jpgis excluded by!**/*.jpg
📒 Files selected for processing (4)
MANIFEST.in(1 hunks)__init__.py(1 hunks)requirements.txt(1 hunks)skill.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- MANIFEST.in
🔇 Additional comments (2)
requirements.txt (1)
5-5: Verify compatibility with existing OVOS components
The addition of ovos-bus-client>=1.0.1 looks good, but let's verify compatibility with other OVOS components.
✅ Verification successful
The verification shows that ovos-bus-client>=1.0.1 is consistently specified in both skill.json and requirements.txt, and the setup.py reads requirements directly from requirements.txt. There are no conflicting version specifications across the codebase.
Compatible version specification confirmed
The version constraint ovos-bus-client>=1.0.1 is consistently defined across the codebase with no conflicts found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential version conflicts and compatibility issues
# Check if any other files specify different version constraints for ovos-bus-client
rg -l "ovos-bus-client" | while read -r file; do
echo "=== $file ==="
rg "ovos-bus-client" "$file"
done
# Look for any dependency specifications in setup files
fd -e py setup.py | while read -r setup; do
echo "=== $setup ==="
rg "install_requires|dependencies" "$setup" -A 5
done
Length of output: 686
skill.json (1)
17-17: LGTM: New dependency addition looks appropriate.
The addition of ovos-bus-client>=1.0.1 aligns with the modernization objective.
Let's verify the compatibility of this version with the project's supported Python versions:
✅ Verification successful
Dependency addition verified and compatible
The addition of ovos-bus-client>=1.0.1 is appropriate:
- Latest version on PyPI is 1.0.1, matching the minimum version requirement
- No explicit Python version constraints in the package or its dependencies
- Follows the same dependency pattern as other OVOS components in requirements.txt
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check ovos-bus-client compatibility and latest version
# Check PyPI for latest versions and Python compatibility
curl -s https://pypi.org/pypi/ovos-bus-client/json | jq -r '.info | {version: .version, python_requires: .requires_python}'
Length of output: 177
Script:
#!/bin/bash
# Let's check the Python version requirements in setup.py or pyproject.toml
fd "setup.py|pyproject.toml" --exec cat {}
Length of output: 3168
Script:
#!/bin/bash
# Let's check the requirements.txt file for any Python version constraints
cat requirements.txt
Length of output: 101
Summary by CodeRabbit
New Features
ovos-bus-clientto improve functionality.Bug Fixes
LaughSkillclass.Documentation
hauntedproperty in theLaughSkillclass.sounds_dirproperty in theLaughSkillclass.