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.

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

Allow "everyday" #19

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Allow "everyday" #19

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
196b565
Allow "everyday" as a day to schedule
Jan 14, 2022
90306f2
Remove debug logging
Jan 14, 2022
9823c25
Merge branch 'master' of https://github.com/burnsfisher/mycroft_routi…
Jan 14, 2022
6a783b6
Merge branch 'ChristopherRogers1991:master' into master
burnsfisher Jan 14, 2022
7e82f2d
This does not work
Jan 15, 2022
8f05054
Fix problems suggested by @Christoperrogers1991:
Jan 15, 2022
c162388
Add new files
Jan 15, 2022
48f8aba
Merge branch 'master' of https://github.com/burnsfisher/mycroft_routi…
Jan 15, 2022
f6896b8
Improve error finding and exception handling for routine scheduling.
Jan 16, 2022
1734e75
One typo fix that I missed
Jan 16, 2022
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
6 changes: 5 additions & 1 deletion __init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,11 @@ def _get_days(self):
days_from_user = days_from_user.lower()
for i in range(len(self._days_of_week)):
if self._days_of_week[i] in days_from_user:
days_to_run.append(str(i))
self.log.info(days_from_user+str(i))
if i == 7:
days_to_run = ['0','1','2','3','4','5','6']
Copy link
Owner

@ChristopherRogers1991 ChristopherRogers1991 Jan 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think this implementation could result in some issues - for example, if the user responds "every Wednesday". I think we only want to look for "every" if there were no other days present. So if days_to_run is empty, check if "every" is present. We would probably also want to check for "everyday", so we could make a new voc file with both of those options, and check if any are present.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I've fixed these things. I'm not sure "everyday" is needed since it does match "every", but regardless, I added it. I also made sure that "every" is not counted as day number 7 or 8 if it is specified with with another day, nor does it replace the days if it is spoken along with other days.
In the process of debugging, I found some easy-to-generate errors that can leave the routines file corrupted (which can prevent routines from loading) so I put in some error traps.
I think my new stuff gets included in this pull request, but I'm not 100% sure. You can check the German days of week to be sure that "Jedentag" is removed to confirm that my changes made it in. BTW, I also added a new dialog for a new error (Unknown day) and just left that file blank in German.

Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this is quite what we want either. The reason for suggesting a separate voc file in my initial comment was because it allows us to have a different number of terms per language, without the loop logic breaking. We can expect every language to have one name for each day of the week, so one file and a loop works there (though this isn't actually the best design - more on this later*). But for "every day," there might be multiple different ways of saying that, and the number might be different per language. For example, in English, we should cover 'every day' and 'everyday' (different speech to text engines may give different results, and as they evolve overtime, even the same STT engine could produce different results). Even though that problem is somewhat unique to English, lets assume all other languages support 2 ways of saying 'every day.' So we write the loop to handle 1-9 and we call it good. But then in the future, we might want add 'all days' and 'each day' to the English version, as alternative ways to say 'every day.' This gets us in a slightly precarious spot, because we have different numbers of terms per language now. It's not so bad if we add nothing else - we can just logic around it by saying anything > 7 is 'every,' but what about if later we decide to add 'weekdays' and 'weekends' as ways to say Mon-Fri and Sat-Sun respectively? Now that greater than logic no longer works. We can keep pushing the 'every' terms to the end, but that assumes all other terms always grow at the same rate - e.g. we might want to add 'weeknights' as an equivalent term to `weekdays', and other languages might not have a distinction between those terms.

This is a very long winded way to say that there are different ways to say the same thing, and if we put all the terms for all the day combinations in one file, we have to make sure there are the same number of terms, for each combination of days, for each language. Otherwise, the loop logic breaks.

*Ideally (though this isn't something we should necessarily tackle in this PR), even the days of the week would be split into their own files. This would allow us to also add colloquial terms for those days, e.g. 'hump day' for Wednesday.

Bottom line - I think we want a separate Everyday.voc file, containing 'every day' and 'everyday', and then we want to break the check for that out from the loop, and check for those explicitly.

Let me know if that makes sense.

else:
days_to_run.append(str(i))
return ','.join(days_to_run)

def _get_time(self):
Expand Down
1 change: 1 addition & 0 deletions vocab/de-de/DaysOfWeek.voc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@ Donnerstag
Freitag
Samstag
Sonntag
Jedentag
Copy link
Owner

Choose a reason for hiding this comment

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

I don't speak German either - perhaps @gras64 can give us a confirmation on the translation. Otherwise, I'd leave this out (I'd rather not have it than have it be incorrect, since it's harder to change than to add - changing breaks things for people who work around incorrectness)

1 change: 1 addition & 0 deletions vocab/en-us/DaysOfWeek.voc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@ thursday
friday
saturday
sunday
every