Skip to content

[BG] Add HassGetDate and HassGetTime #2432

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Nov 13, 2024
Merged

[BG] Add HassGetDate and HassGetTime #2432

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
87e4cd8
[BG] Add HassGetDate and HassGetTime
kyutov Oct 28, 2024
b4356af
Fixed typo
kyutov Oct 28, 2024
ca9b55b
Change the names of all numbers in HassGetCurrentDate.yaml
hristo-atanasov Nov 1, 2024
8c8294c
Add more variants in homeassistant_HassGetCurrentDate.yaml
hristo-atanasov Nov 1, 2024
f60e6e2
Fix indentatios homeassistant_HassGetCurrentDate.yaml
hristo-atanasov Nov 2, 2024
39acbcd
Add more tests in homeassistant_HassGetCurrentDate.yaml
hristo-atanasov Nov 2, 2024
3d61b5c
Fix the response to match the new date names homeassistant_HassGetCur…
hristo-atanasov Nov 2, 2024
de61114
Add weekday in HassGetCurrentDate response
kyutov Nov 4, 2024
ae0795c
Improved responses in HassGetCurrentDate and HassGetCurrentTime
kyutov Nov 5, 2024
f71ee90
Delete responses/bg/HassGetCurrentTime.yaml
hristo-atanasov Nov 13, 2024
d7a7408
Delete sentences/bg/homeassistant_HassGetCurrentTime.yaml
hristo-atanasov Nov 13, 2024
2f93c97
Delete tests/bg/homeassistant_HassGetCurrentTime.yaml
hristo-atanasov Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions responses/bg/HassGetCurrentDate.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
language: bg
responses:
intents:
HassGetCurrentDate:
default: >
{% set months = {
1: 'Януари',
2: 'Февруари',
3: 'Март',
4: 'Април',
5: 'Май',
6: 'Юни',
7: 'Юли',
8: 'Август',
9: 'Септември',
10: 'Октомври',
11: 'Ноември',
12: 'Декември',
} %}
{% set ordinal = {
1: 'първи',
2: 'втори',
3: 'трети',
4: 'четвърти',
5: 'пети',
6: 'шести',
7: 'седми',
8: 'осми',
9: 'девети',
10: 'десети',
11: 'единадесети',
12: 'дванадесети',
13: 'тринадесети',
14: 'четиринадесети',
15: 'петнадесети',
16: 'шестнадесети',
17: 'седемнадесети',
18: 'осемнадесети',
19: 'деветнадесети',
20: 'двадесети',
21: 'двадесет и първи',
22: 'двадесет и втори',
23: 'двадесет и трети',
24: 'двадесет и четвърти',
25: 'двадесет и пети',
26: 'двадесет и шести',
27: 'двадесет и седми',
28: 'двадесет и осми',
29: 'двадесет и девети',
30: 'тридесети',
31: 'тридесет и първи',
} %}
{{ ordinal[slots.date.day] }} {{ months[slots.date.month] }} {{ slots.date.year }}-та
8 changes: 8 additions & 0 deletions responses/bg/HassGetCurrentTime.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
language: bg
responses:
intents:
HassGetCurrentTime:
default: >
{% set hour_str = '{0:02d}'.format(slots.time.hour) %}
{% set minute_str = '{0:02d}'.format(slots.time.minute) %}
{{ hour_str }} часа и {{ minute_str }} минути
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

🛠️ Refactor suggestion

The review suggestions are valid and should be implemented

After analyzing other language implementations, the review suggestions are well-founded:

  1. None of the implementations handle missing time slots, making error handling a valid concern
  2. Several languages (es, pt, ro) implement proper grammatical rules for their time units
  3. Some implementations (es) handle special cases like noon/midnight
  4. The Bulgarian implementation needs these improvements for better localization

The suggested improvements in the review are appropriate and align with best practices seen in other language implementations, particularly:

  • Error handling for missing slots
  • Proper grammar rules for "час"/"часа"
  • Special cases for noon/midnight
🔗 Analysis chain

Consider improving time formatting and adding error handling.

A few suggestions to enhance the implementation:

  1. Add error handling for missing time slots
  2. Consider Bulgarian grammar rules for hours:
    • Use "час" for 1 hour
    • Use "часа" for 2-24 hours
  3. Consider special cases like noon (обяд) and midnight (полунощ)

Here's a suggested improvement:

 default: >
+  {%- if not slots.time %}
+    Съжалявам, но не мога да определя часа.
+  {%- else %}
   {% set hour_str = '{0:02d}'.format(slots.time.hour) %}
   {% set minute_str = '{0:02d}'.format(slots.time.minute) %}
-  {{ hour_str }} часа и {{ minute_str }} минути
+  {%- if slots.time.hour == 12 and slots.time.minute == 0 %}
+    обяд
+  {%- elif slots.time.hour == 0 and slots.time.minute == 0 %}
+    полунощ
+  {%- else %}
+    {{ hour_str }} {% if slots.time.hour == 1 %}час{% else %}часа{% endif %} и {{ minute_str }} минути
+  {%- endif %}
+  {%- endif %}

Let's verify if there are any similar patterns in other language implementations:

Let me gather more information about error handling patterns in other language implementations.

Let me check the full content of time response files to understand the complete implementation patterns.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check how other languages handle noon/midnight and hour pluralization
rg -g "responses/**/HassGetCurrentTime.yaml" -A 10 "hour.*format"

Length of output: 1556

Script:

#!/bin/bash
# Check for error handling patterns in time responses
rg -g "responses/**/HassGetCurrentTime.yaml" -B 2 -A 5 "if not slots.time"

Length of output: 76

Script:

#!/bin/bash
# Get full content of time response files
rg -g "responses/**/HassGetCurrentTime.yaml" --no-line-number "^" -A 20

Length of output: 21405

9 changes: 9 additions & 0 deletions sentences/bg/homeassistant_HassGetCurrentDate.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
language: bg

Check failure on line 1 in sentences/bg/homeassistant_HassGetCurrentDate.yaml

View workflow job for this annotation

GitHub Actions / build 

invalid YAML: while scanning a simple key
intents:
HassGetCurrentDate:
data:
- sentences:
- "[кажи ми] [коя е|каква е] [днешната] дата[та]"
- "[кажи ми] (коя|каква) дата (е|сме) [днес]"
+ - "[кажи ми] датата [днес]"
+ - "[кажи ми] кой ден сме [днес]"
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix YAML syntax errors in sentence patterns.

The indentation in lines 8-9 is incorrect, causing YAML parsing errors. Also, there's inconsistent use of grouping symbols.

Apply this diff to fix the syntax and make the patterns consistent:

    data:
      - sentences:
          - "[кажи ми] [коя е|каква е] [днешната] дата[та]"
          - "[кажи ми] (коя|каква) дата (е|сме) [днес]"
-  +       - "[кажи ми] датата [днес]"
-  +       - "[кажи ми] кой ден сме [днес]"
+         - "[кажи ми] датата [днес]"
+         - "[кажи ми] кой ден сме [днес]"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
data:
- sentences:
- "[кажи ми] [коя е|каква е] [днешната] дата[та]"
- "[кажи ми] (коя|каква) дата (е|сме) [днес]"
+ - "[кажи ми] датата [днес]"
+ - "[кажи ми] кой ден сме [днес]"
data:
- sentences:
- "[кажи ми] [коя е|каква е] [днешната] дата[та]"
- "[кажи ми] (коя|каква) дата (е|сме) [днес]"
- "[кажи ми] датата [днес]"
- "[кажи ми] кой ден сме [днес]"
🧰 Tools
🪛 yamllint

[error] 9-9: syntax error: could not find expected ':'

(syntax)

7 changes: 7 additions & 0 deletions sentences/bg/homeassistant_HassGetCurrentTime.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
language: bg
intents:
HassGetCurrentTime:
data:
- sentences:
- "[кажи ми] колко е часа"
- "[кажи ми] час[а]"
11 changes: 11 additions & 0 deletions tests/bg/homeassistant_HassGetCurrentDate.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
language: bg
tests:
- sentences:
- "дата"
- "кажи ми датата"
- "коя е днешната дата"
- "коя дата е днес"
- "кажи ми днешната дата"
intent:
name: HassGetCurrentDate
response: "седемнайсети Септември 2013-та"
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

Replace hardcoded response with dynamic date formatting - Update needed across all languages

Looking at the test files across all languages, I can see that all of them are using hardcoded dates (specifically September 17th, 2013) in their test responses. This is a systemic pattern that needs to be addressed. The review comment is correct, but the scope should be expanded:

  • All language test files need to be updated to use dynamic date formatting
  • The suggested format {% now 'j F Y' %} would need to be adapted for each language's specific date formatting requirements
  • The Bulgarian (bg) implementation should maintain its ordinal number formatting system (as seen in the response file with ordinal mapping)
🔗 Analysis chain

Replace hardcoded response with dynamic date formatting.

The current hardcoded response "седемнайсети Септември 2013-та" is problematic:

  1. It uses a static date from 2013
  2. It doesn't match the expected behavior of returning the current date

Consider using a reference date format like this:

-    response: "седемнайсети Септември 2013-та"
+    response: "{% now 'j F Y' %}"

Let's verify if other language test files use dynamic dates:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for date-related test responses in other language files
rg -g 'tests/*/homeassistant_HassGetCurrentDate.yaml' 'response:'

Length of output: 1587

Script:

#!/bin/bash
# Search for date-related test responses in other language files using a more specific pattern
rg -g 'tests/*/homeassistant_HassGetCurrentDate.yaml' -l '.'

# Then read the content of each found file
fd 'homeassistant_HassGetCurrentDate.yaml' tests/ --exec cat {}

Length of output: 5716

10 changes: 10 additions & 0 deletions tests/bg/homeassistant_HassGetCurrentTime.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
language: bg
tests:
- sentences:
- "колко е часа"
- "кажи ми колко е часа"
- "час"
- "кажи ми час"
intent:
name: HassGetCurrentTime
response: "01 часа и 02 минути"
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance test coverage and response format.

While the basic test cases are good, consider the following improvements:

  1. Add more test cases to cover:

    • Different hour formats (e.g., "13 часа" vs "1 часа")
    • Edge cases like midnight ("полунощ") and noon ("обяд")
    • Different time formats (24-hour vs 12-hour)

  2. The hard-coded response "01 часа и 02 минути" is too specific. Consider using a more generic format that can handle:

    • Hours 1-24
    • Minutes 0-59
    • Proper Bulgarian grammar for different numbers (e.g., "1 час" vs "2 часа")

Here's a suggested expansion of test cases:

tests:
  - sentences:
      - "колко е часа"
      - "кажи ми колко е часа"
      - "час"
      - "кажи ми час"
      - "колко е часът"
      - "в колко часа сме"
    intent:
      name: HassGetCurrentTime
    response: "01 часа и 02 минути"
  - sentences:
      - "колко е часа"
    intent:
      name: HassGetCurrentTime
    response: "13 часа и 00 минути"
  - sentences:
      - "колко е часа"
    intent:
      name: HassGetCurrentTime
    response: "00 часа и 00 минути"

Loading