Skip to content
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

feat: hour chime #66

Merged
merged 6 commits into from
Nov 12, 2024
Merged

feat: hour chime #66

merged 6 commits into from
Nov 12, 2024

Conversation

JarbasAl
Copy link
Member

@JarbasAl JarbasAl commented Nov 6, 2024

closes #47

Summary by CodeRabbit

  • New Features

    • Introduced a "Configuration" section in the README for adjusting skill behavior via settings.json.
    • Added options for hourly chime notifications and customizable chime sound.
  • Bug Fixes

    • Improved error handling for timezone retrieval methods, allowing for optional returns.
  • Documentation

    • Updated README to include new configuration options and example usage, along with updated credits for audio files.
  • Chores

    • Enhanced resource file handling to include MP3 files in the package data.

Copy link

coderabbitai bot commented Nov 6, 2024

Warning

Rate limit exceeded

@JarbasAl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 34 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

📥 Commits

Reviewing files that changed from the base of the PR and between ab6d3f3 and 5ab0a20.

Walkthrough

The pull request introduces enhancements to the README.md, __init__.py, and setup.py files. The README.md now includes a new "Configuration" section detailing user-customizable options, such as sound file settings. The __init__.py file updates the TimeSkill class to improve time management functionalities, including new methods for handling hourly chimes and adjustments to existing method signatures to reflect optional return types. The setup.py file modifications expand resource file handling to include additional directories and file types.

Changes

File Change Summary
README.md Added "Configuration" section with details on settings.json options for chime settings and sound files. Expanded "Credits" section for sound file attributions.
init.py Updated TimeSkill class with new methods for chime management, added properties for chime settings, and modified method signatures to indicate optional return types.
setup.py Modified find_resource_files function to include a new directory "res" and updated package_data to include "*.mp3" files.

Assessment against linked issues

Objective Addressed Explanation
Play sound when hour changes (#47)

Possibly related PRs

  • Feat/seperate mk1 #53: The changes in the __init__.py file related to the TimeSkill class involve modifications to methods that handle time display, which may interact with the new configuration options for chimes and sound files introduced in the main PR.

Poem

In the land of time, where chimes do play,
A rabbit hops forth, to brighten the day.
With sounds of the hour, a joyful delight,
Configuration made easy, all set up just right!
So let’s celebrate changes, both big and small,
For every tick-tock brings joy to us all! 🐇✨


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (9)
README.md (4)

23-28: Show default values in the configuration example.

The current example uses a custom path that might not be immediately useful to users. Consider showing the default configuration instead:

 {
     "play_hour_chime": true,
-    "hour_sound": "/path/to/your/custom/sound.mp3"
+    "hour_sound": "res/clock-chime-88027.mp3"
 }

30-32: Use consistent unordered list style markers.

For consistency with Markdown standards, use asterisks (*) instead of dashes (-) for unordered lists.

- - **`play_hour_chime`**: (boolean) Enables or disables...
- - **`hour_sound`**: (string) Specifies the file path...
+ * **`play_hour_chime`**: (boolean) Enables or disables...
+ * **`hour_sound`**: (string) Specifies the file path...
🧰 Tools
🪛 Markdownlint

30-30: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


32-32: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


32-32: Clarify accepted audio file formats and paths.

The hour_sound description should specify:

  1. Supported audio formats (MP3 only?)
  2. Whether absolute paths are supported
  3. Base path for relative paths
🧰 Tools
🪛 Markdownlint

32-32: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


35-37: Fix list style but content LGTM!

The credit information is complete with proper attribution and licensing. Just update the list style for consistency:

- - [clock-chime.mp3 by @ecfike]...
- - Original skill by Mycroft AI (@MycroftAI)
+ * [clock-chime.mp3 by @ecfike]...
+ * Original skill by Mycroft AI (@MycroftAI)
🧰 Tools
🪛 Markdownlint

36-36: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


37-37: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

__init__.py (5)

68-74: Improve the docstring.

The docstring for the initialize method could be more concise and focused on the key aspects of what the method does. Consider refining it to something like:

"""Initialize the skill by preloading language settings and scheduling the hourly chime."""

This captures the essence of the method's purpose without going into unnecessary details about when it's called or what the chime scheduling entails.


92-101: Consider using datetime.timedelta for better readability.

Instead of manually constructing the datetime object for the next hour, you can use datetime.timedelta to add one hour to the current time. This makes the code more readable and less error-prone.

def _schedule_hour_chime(self):
    n = now_local() + datetime.timedelta(hours=1)
    self.schedule_event(self._schedule_hour_chime,
                        when=datetime.datetime(year=n.year, month=n.month, day=n.day,
                                               hour=n.hour, minute=0, second=0))

147-148: Update the return type hint to Optional[datetime.tzinfo].

Since the _get_timezone_from_builtins method can return None if the timezone is not found, the return type hint should be updated to reflect this. Change it from datetime.tzinfo to Optional[datetime.tzinfo].

@staticmethod
def _get_timezone_from_builtins(location_string: str) -> Optional[datetime.tzinfo]:

183-183: Update the return type hint to Optional[datetime.tzinfo].

Similar to the previous comment, update the return type hint of _get_timezone_from_fuzzymatch to Optional[datetime.tzinfo] to indicate that it can return None.

def _get_timezone_from_fuzzymatch(self, location_string: str) -> Optional[datetime.tzinfo]:

240-240: Update the return type hint to Optional[datetime.datetime].

Since the get_datetime method can return None if the timezone is not found, update the return type hint to Optional[datetime.datetime] to reflect this.

def get_datetime(self, location: str = None,
                 anchor_date: datetime.datetime = None) -> Optional[datetime.datetime]:
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 766a215 and 0a485bf.

⛔ Files ignored due to path filters (1)
  • res/clock-chime-88027.mp3 is excluded by !**/*.mp3
📒 Files selected for processing (3)
  • README.md (1 hunks)
  • __init__.py (7 hunks)
  • setup.py (1 hunks)
🧰 Additional context used
🪛 Markdownlint
README.md

30-30: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


32-32: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


36-36: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


37-37: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

🔇 Additional comments (7)
README.md (1)

19-22: LGTM! Clear and informative section introduction.

The configuration section introduction effectively explains the purpose and location of the configuration options.

setup.py (1)

33-33: LGTM! Verify resource packaging.

The changes correctly enable MP3 file packaging for the hourly chime feature.

Let's verify the resource structure and file inclusion:

Also applies to: 35-35

✅ Verification successful

Resource packaging setup is correctly configured

The verification confirms:

  • MP3 file clock-chime-88027.mp3 exists in the res directory
  • MANIFEST.in includes res directory with recursive-include res * directive
  • Setup matches the changes in setup.py
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the existence of MP3 resources and their packaging

# Test 1: Check if the res directory exists and contains MP3 files
echo "Checking for MP3 files in res directory..."
fd -e mp3 -d 1 . res/

# Test 2: Verify the specific chime file mentioned in the PR
echo "Checking for the specific clock chime file..."
fd "clock-chime-88027.mp3" .

# Test 3: Ensure MANIFEST.in includes res directory (if exists)
echo "Checking MANIFEST.in configuration..."
if [ -f "MANIFEST.in" ]; then
    cat MANIFEST.in
fi

Length of output: 613

__init__.py (5)

15-15: LGTM!

The addition of the os import is necessary for the new functionality related to the hourly chime sound file path.


78-91: LGTM!

The _handle_play_hour_chime method is well-structured and properly handles playing the chime sound based on the play_hour_chime setting. The re-scheduling of the next chime event is also correctly implemented.


103-120: LGTM!

The play_hour_chime and hour_chime properties are well-defined and provide clear access to the relevant settings. The default value for hour_chime is also properly set using the os.path.dirname to construct the file path relative to the skill's directory.


502-502: LGTM!

The formatting change in the emitted message for displaying the date on the Mark 1 is minor and doesn't affect the functionality.


526-526: LGTM!

The formatting change in the emitted message for displaying the time on the Mark 1 is minor and doesn't affect the functionality.

@github-actions github-actions bot added feature and removed feature labels Nov 6, 2024
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (3)
README.md (2)

16-32: Consider enhancing configuration documentation.

The configuration section is well-structured, but could benefit from additional details:

  1. Specify supported audio file formats
  2. Provide guidance on where users should place custom sound files
  3. Document any file size limitations or recommendations
 - **`hour_sound`**: (string) Specifies the file path to the audio file used for the hourly chime. By default, it points to `casio-watch.wav` in the `res` folder. You can customize this with the path to any audio file you prefer.
+- **`hour_sound`**: (string) Specifies the file path to the audio file used for the hourly chime. By default, it points to `casio-watch.wav` in the `res` folder. Supports WAV and MP3 formats. For custom sounds, place your audio files in the skill's `res` directory and specify the filename. Keep file sizes reasonable (< 1MB recommended) for optimal performance.
🧰 Tools
🪛 Markdownlint

31-31: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


32-32: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


31-32: Maintain consistent list style.

For better markdown consistency, use asterisks (*) instead of dashes (-) for unordered lists throughout the document.

-    - **`play_hour_chime`**: (boolean) Enables or disables the hourly chime notification...
-    - **`hour_sound`**: (string) Specifies the file path to the audio file...
+    * **`play_hour_chime`**: (boolean) Enables or disables the hourly chime notification...
+    * **`hour_sound`**: (string) Specifies the file path to the audio file...

-    - [casio-watch.wav by @Pablobd]...
-    - [clock-chime.mp3 by @ecfike]...
-    - Original skill by Mycroft AI (@MycroftAI)
+    * [casio-watch.wav by @Pablobd]...
+    * [clock-chime.mp3 by @ecfike]...
+    * Original skill by Mycroft AI (@MycroftAI)

Also applies to: 36-38

🧰 Tools
🪛 Markdownlint

31-31: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


32-32: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

__init__.py (1)

121-123: Use os.path.join for cross-platform path construction

To improve cross-platform compatibility and code readability, consider using os.path.join when constructing file paths instead of concatenating strings with slashes. This ensures that the code functions correctly on different operating systems.

Apply this diff to refactor the path construction:

 if not os.path.isfile(snd):
-    snd2 = f"{os.path.dirname(__file__)}/res/{snd}"
+    snd2 = os.path.join(os.path.dirname(__file__), "res", snd)
     snd = snd2 if os.path.isfile(snd2) else snd
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 766a215 and e23bb1c.

⛔ Files ignored due to path filters (2)
  • res/casio-watch.wav is excluded by !**/*.wav
  • res/clock-chime.mp3 is excluded by !**/*.mp3
📒 Files selected for processing (3)
  • README.md (2 hunks)
  • __init__.py (7 hunks)
  • setup.py (1 hunks)
🧰 Additional context used
🪛 Markdownlint
README.md

31-31: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


32-32: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


36-36: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


37-37: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


38-38: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

🔇 Additional comments (4)
README.md (2)

5-5: LGTM! Clear and concise description.


35-38: LGTM! Proper attribution and licensing information.

Excellent job providing comprehensive credits with proper attribution and license links for the sound files.

🧰 Tools
🪛 Markdownlint

36-36: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


37-37: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


38-38: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

setup.py (2)

33-33: LGTM! Good practice to separate resource files.

The addition of "res" to resource_base_dirs follows good practice by keeping media files separate from code and configuration.


35-35: LGTM! Verify sound file inclusion.

Adding "*.mp3" to package_data is necessary for the hour chime feature.

Let's verify the sound file exists in the correct location:

✅ Verification successful

Sound file inclusion verified successfully

The clock-chime.mp3 file exists in the expected res directory, confirming that the addition of "*.mp3" to package_data in setup.py is appropriate and necessary for the hour chime feature.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the existence and location of MP3 files
# Expected: Find at least one MP3 file in the res directory for the hour chime

# Check for MP3 files in the res directory
echo "Checking for MP3 files in res directory..."
fd -e mp3 -p res/

# Verify if the specific clock chime file exists
echo "Checking for clock chime sound file..."
fd "clock-chime.mp3" -p res/

Length of output: 270

__init__.py Outdated Show resolved Hide resolved
@github-actions github-actions bot added feature and removed feature labels Nov 6, 2024
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (3)
README.md (2)

16-32: Documentation improvements needed for Configuration section

While the configuration options are well documented, please consider the following improvements:

  1. Specify the location of the settings.json file
  2. Make the default values more prominent in the initial description

Consider updating the introduction like this:

 ## Configuration
 
-You can adjust certain aspects of this skill's behavior by configuring the `settings.json` file. 
+You can adjust certain aspects of this skill's behavior by configuring the `~/.config/mycroft/skills/skill-ovos-date-time/settings.json` file. 
 
 2 sound files are included with the skill, `"casio-watch.wav"` and `"clock-chime.mp3"`, to audibly signal when the hour changes
 
-Below is an example configuration file with explanations for each option.
+Below is an example configuration file with explanations for each option. By default, hourly chimes are disabled (`play_hour_chime: false`) and use `casio-watch.wav`.
🧰 Tools
🪛 Markdownlint

31-31: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


32-32: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


35-38: Fix list marker style inconsistency

The unordered list uses dash (-) markers while other lists in the document use asterisks (*). Let's maintain consistency.

Update the list markers:

-Credits 
+## Credits 
 
-- [casio-watch.wav by @Pablobd](https://freesound.org/people/Pablobd/sounds/492481/) under the [CC0 1.0 Universal License](https://creativecommons.org/publicdomain/zero/1.0/)
-- [clock-chime.mp3 by @ecfike](https://pixabay.com/sound-effects/clock-chime-88027/) under the [Pixabay Content License](https://pixabay.com/service/license-summary/)
-- Original skill by Mycroft AI (@MycroftAI)
+* [casio-watch.wav by @Pablobd](https://freesound.org/people/Pablobd/sounds/492481/) under the [CC0 1.0 Universal License](https://creativecommons.org/publicdomain/zero/1.0/)
+* [clock-chime.mp3 by @ecfike](https://pixabay.com/sound-effects/clock-chime-88027/) under the [Pixabay Content License](https://pixabay.com/service/license-summary/)
+* Original skill by Mycroft AI (@MycroftAI)
🧰 Tools
🪛 Markdownlint

36-36: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


37-37: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


38-38: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

__init__.py (1)

99-101: Fix indentation in schedule_event call.

The indentation is inconsistent with the rest of the codebase.

-        self.schedule_event(self._schedule_hour_chime,
-                            when=datetime.datetime(year=n.year, month=n.month, day=n.day,
-                                                   hour=n.hour, minute=0, second=0))
+        self.schedule_event(
+            self._schedule_hour_chime,
+            when=datetime.datetime(
+                year=n.year, month=n.month, day=n.day,
+                hour=n.hour, minute=0, second=0
+            )
+        )
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 0a485bf and e23bb1c.

⛔ Files ignored due to path filters (2)
  • res/casio-watch.wav is excluded by !**/*.wav
  • res/clock-chime.mp3 is excluded by !**/*.mp3
📒 Files selected for processing (2)
  • README.md (2 hunks)
  • __init__.py (7 hunks)
🧰 Additional context used
🪛 Markdownlint
README.md

31-31: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


32-32: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


36-36: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


37-37: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


38-38: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

🔇 Additional comments (4)
__init__.py (4)

68-76: LGTM! Well documented initialization.

The initialize method is well structured with clear documentation and proper setup of language settings and hourly chime scheduling.


78-90: LGTM! Clean implementation of hourly chime handler.

The implementation correctly handles the chime playback and rescheduling. Using instant=True ensures accurate timing of the chime.


151-152: LGTM! Improved type safety with Optional returns.

The changes properly handle cases where timezone lookup fails by using Optional return types. This is a good practice that helps prevent runtime errors.

Also applies to: 187-187, 244-244


506-506: LGTM! Consistent indentation in display methods.

The indentation changes in the message emission calls improve code consistency.

Also applies to: 530-530

__init__.py Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (5)
README.md (3)

16-32: Enhance configuration documentation with additional details.

While the configuration section is well-structured, consider adding:

  1. Supported audio file formats
  2. Whether relative/absolute paths are supported for custom sounds
  3. Location where custom sound files should be placed
🧰 Tools
🪛 Markdownlint

31-31: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


32-32: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


31-32: Fix markdown list style consistency.

Use asterisks (*) instead of dashes (-) for unordered lists to maintain consistency with the rest of the document.

-    - **`play_hour_chime`**: (boolean) Enables or disables the hourly chime notification. If `true`, the skill will play an audio chime at the start of every hour. Default is `false`.
-    - **`hour_sound`**: (string) Specifies the file path to the audio file used for the hourly chime. By default, it points to `casio-watch.wav` in the `res` folder. You can customize this with the path to any audio file you prefer.
+    * **`play_hour_chime`**: (boolean) Enables or disables the hourly chime notification. If `true`, the skill will play an audio chime at the start of every hour. Default is `false`.
+    * **`hour_sound`**: (string) Specifies the file path to the audio file used for the hourly chime. By default, it points to `casio-watch.wav` in the `res` folder. You can customize this with the path to any audio file you prefer.
🧰 Tools
🪛 Markdownlint

31-31: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


32-32: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


36-38: Fix markdown list style consistency in credits section.

Use asterisks (*) instead of dashes (-) for unordered lists to maintain consistency.

-    - [casio-watch.wav by @Pablobd](https://freesound.org/people/Pablobd/sounds/492481/) under the [CC0 1.0 Universal License](https://creativecommons.org/publicdomain/zero/1.0/)
-    - [clock-chime.mp3 by @ecfike](https://pixabay.com/sound-effects/clock-chime-88027/) under the [Pixabay Content License](https://pixabay.com/service/license-summary/)
-    - Original skill by Mycroft AI (@MycroftAI)
+    * [casio-watch.wav by @Pablobd](https://freesound.org/people/Pablobd/sounds/492481/) under the [CC0 1.0 Universal License](https://creativecommons.org/publicdomain/zero/1.0/)
+    * [clock-chime.mp3 by @ecfike](https://pixabay.com/sound-effects/clock-chime-88027/) under the [Pixabay Content License](https://pixabay.com/service/license-summary/)
+    * Original skill by Mycroft AI (@MycroftAI)
🧰 Tools
🪛 Markdownlint

36-36: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


37-37: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


38-38: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

__init__.py (2)

Line range hint 248-255: Handle None return from get_datetime in get_spoken_time.

Since get_datetime may now return None, get_spoken_time should handle this case to prevent exceptions when dt is None. Please add a check for dt and handle the situation appropriately, possibly by returning an error message or a default time.

Apply this diff to handle the None case:

 def get_spoken_time(self, location: str = None, force_ampm=False,
                     anchor_date: datetime.datetime = None) -> str:
     """Get formatted spoken time based on user preferences."""
     dt = self.get_datetime(location, anchor_date)
+    if dt is None:
+        return self.translate("time.tz.not.found", {"location": location}) if location else None
     # speak AM/PM when talking about somewhere else
     say_am_pm = bool(location) or force_ampm

Line range hint 265-273: Handle None return from get_datetime in get_display_time.

Similar to get_spoken_time, the get_display_time method should handle cases where get_datetime returns None to prevent exceptions.

Apply this diff to handle the None case:

 def get_display_time(self, location: str = None, force_ampm=False,
                      anchor_date: datetime.datetime = None) -> str:
     """Get formatted display time based on user preferences."""
     dt = self.get_datetime(location, anchor_date)
+    if dt is None:
+        return None  # Or handle appropriately
     # speak AM/PM when talking about somewhere else
     say_am_pm = bool(location) or force_ampm
     return nice_time(dt, lang=self.lang,
                      speech=False,
                      use_24hour=self.use_24hour,  # session aware
                      use_ampm=say_am_pm)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 766a215 and e23bb1c.

⛔ Files ignored due to path filters (2)
  • res/casio-watch.wav is excluded by !**/*.wav
  • res/clock-chime.mp3 is excluded by !**/*.mp3
📒 Files selected for processing (3)
  • README.md (2 hunks)
  • __init__.py (7 hunks)
  • setup.py (1 hunks)
🧰 Additional context used
🪛 Markdownlint
README.md

31-31: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


32-32: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


36-36: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


37-37: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


38-38: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

🔇 Additional comments (3)
README.md (2)

5-5: LGTM! Improved readability of the basic functionality description.


Line range hint 1-38: Documentation successfully implements hour chime feature requirements.

The README changes comprehensively document the new hour chime feature, including configuration options and proper attribution for sound files. This aligns perfectly with the PR objectives from issue #47.

🧰 Tools
🪛 Markdownlint

31-31: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


32-32: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


36-36: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


37-37: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


38-38: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


41-41: null
Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

setup.py (1)

33-33: LGTM! Verify resource directory structure.

The changes correctly add support for MP3 files and the "res" directory, which aligns with the hour chime feature requirements.

Let's verify the resource directory structure:

Also applies to: 35-35

✅ Verification successful

Resource directory structure verified successfully

The res directory exists and contains the expected clock-chime.mp3 file, confirming proper resource structure for the hour chime feature.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the existence of the res directory and MP3 files

# Test 1: Check if res directory exists
if [ -d "res" ]; then
    echo "✓ res directory exists"
else
    echo "⚠ res directory not found"
fi

# Test 2: Look for MP3 files in res directory
fd -e mp3 -d 1 . res/

Length of output: 115

__init__.py Show resolved Hide resolved
__init__.py Outdated Show resolved Hide resolved
__init__.py Show resolved Hide resolved
@j1nx
Copy link
Member

j1nx commented Nov 9, 2024

Looks like a great new feature. Does anyone has that Casio watch bleep sound? Casio was always doing this for their cheap ass watches you won at the Fairs.

can't really comment on the code itself. Do you want me to test this?

@JarbasAl
Copy link
Member Author

JarbasAl commented Nov 9, 2024

Does anyone has that Casio watch bleep sound?

in the PR i am using this https://freesound.org/people/Pablobd/sounds/492481/

Copy link

@mikejgray mikejgray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build test failing. Code looks good but I'll need to test it in one of my assistants

@JarbasAl
Copy link
Member Author

JarbasAl commented Nov 12, 2024

Build test failing. Code looks good but I'll need to test it in one of my assistants

no clue what "test manifest" is actually testing and how to solve it, first time i'm running into this workflow. anyone care to explain this automation?


nvm, got it. but we should standardize this across our repos, i don't recall seeing this one before!

@mikejgray
Copy link

Build test failing. Code looks good but I'll need to test it in one of my assistants

no clue what "test manifest" is actually testing and how to solve it, first time i'm running into this workflow. anyone care to explain this automation?

nvm, got it. but we should standardize this across our repos, i don't recall seeing this one before!

https://github.com/tj-actions/check-manifest

It's disabled by default, but available in Neon test automations: https://github.com/NeonGeckoCom/.github/blob/09f0850847bf45f02a8cd7774097bb570e6a8fd7/.github/workflows/python_build_tests.yml#L12

@JarbasAl
Copy link
Member Author

Build test failing. Code looks good but I'll need to test it in one of my assistants

no clue what "test manifest" is actually testing and how to solve it, first time i'm running into this workflow. anyone care to explain this automation?
nvm, got it. but we should standardize this across our repos, i don't recall seeing this one before!

tj-actions/check-manifest

It's disabled by default, but available in Neon test automations: NeonGeckoCom/.github@09f0850/.github/workflows/python_build_tests.yml#L12

yeah it's a nice one to have! I just missed it until now somehow. I approve adopting that one across more repos 👍

@JarbasAl JarbasAl merged commit 7dfd939 into dev Nov 12, 2024
8 checks passed
@JarbasAl JarbasAl deleted the feat/chime branch November 12, 2024 06:00
@coderabbitai coderabbitai bot mentioned this pull request Nov 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

play sound - when hour changes
3 participants