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 8 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
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ Create named rountines, which are lists of Mycroft commands, that can be run by
## Credits
* @ChristopherRogers1991
* @gras64 (German translation)

* @burnsfisher (a few bug fixes)

## Short Demos

Expand Down
19 changes: 14 additions & 5 deletions __init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,10 +354,16 @@ def _describe_routine(self, message):

@intent_handler(IntentBuilder("ScheduleRoutine").require("Schedule").require("RoutineName"))
def _add_routine_schedule(self, message):
name = message.data["RoutineName"]
days = self._get_days()
hour, minute = self._get_time()
cronstring = self._generate_cronstring(days, hour, minute)
name = message.data["RoutineName"]
try:
days = self._get_days()
hour, minute = self._get_time()
if days == None or len(days)==0:
self.speak_dialog("unknown.day")
return
cronstring = self._generate_cronstring(days, hour, minute)
except:
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 does what you're expecting. Without the try/except, the code in this method will stop executing if anything throws an unhandled exception (the only way for the except to be triggered here). So, if there is an exception thrown in _get_time, for example, we will never get to the lines where we update the schedule, and write out the file. If you're seeing corruption, the cause cannot be from an exception here, and the try/except will not fix it.

If your file has been corrupted, perhaps you could share the resulting file? That might help track down what went wrong.

Also, just a tip - it's generally better to except Exception, rather than just except. There are certain kinds of exceptions you generally don't want to catch (unless you are explicitly going to handle them), such as keyboard interrupts. See https://stackoverflow.com/a/18982754.

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 looked more carefully. First, thanks for the bit about except Exception. Python is relatively new to me, so anything I can learn is good.

But second, if there was an exception in either get_days or get_time why would you want to update the schedule and write out the file? Something is wrong! Note that a bad time is already caught, and the error spoken in _get_time, at which time it returns None and causes an exception at hour, minute = self._get_time since it is not a list. Nothing caught the exception, and we got an ugly spoken message about an exception in the skill. Similarly, if _get_days did not find a valid day, it returned either None or something with length 0. In this case, I believe cronstring threw an error.

I think that at least one of the corruption cases came if one did not speak the time fast enough, but now I'm not sure. It may be that explicitly catching those errors consistently in the "_get_xxx" routines would be best, but then we'd want to either throw and catch a specific error from them (and return when it was caught) or test the return value for None or len=0. I'd think a try/except would be simpler, but I would probably throw and catch only a value error or something like that and only catch it around the two _get routines.

I'm taking a break now, but I'll a) remove the exception stuff and reproduce the file corruption and b) if I don't hear from you in an hour or 2, implement that error catching as I suggested above (assuming it catches the corruption and exceptions). If you can't respond that soon and don't like what I did, that's ok. I'll re-do it. It's your skill!

Copy link
Owner

Choose a reason for hiding this comment

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

if there was an exception in either get_days or get_time why would you want to update the schedule and write out the file?

We don't/can't. Because it doesn't currently catch the exception, it can never get to the lines where it writes the file. You could show this by editing one of those _get_X methods to always throw an exception, and adding a log line that logs something after _get_X has been called, but before the file is written. You'll see that the exception results in nothing after it being executed, so you won't see your log message.

If you do the try/except, and just return, yes, you will avoid mycroft speaking the message about an error occurring, however, it doesn't change whether the file is written or not. In both cases (with/without the try/except), those lines will not be executed if there is an error.

If the goal is just to prevent the "ugly" message from mycroft on error, that's fine, but we should do something other than just return. E.g. we should log the error, and say something like "I didn't understand that, please try again." Otherwise there is not indication to the user that there was a problem (assuming it's not caught elsewhere).

return
self._routines[name].schedule = cronstring
self._routines[name].enabled = True
self._write_routine_data()
Expand Down Expand Up @@ -390,7 +396,10 @@ 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))
if (i >= 7) and (len(days_to_run)==0) :
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.

elif i < 7:
days_to_run.append(str(i))
return ','.join(days_to_run)

def _get_time(self):
Expand Down
Empty file added dialog/de-de/unknown.day.dialog
View file
Open in desktop
Empty file.
1 change: 1 addition & 0 deletions dialog/en-us/unknown.day.dialog
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
I don't know that day
2 changes: 2 additions & 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,5 @@ thursday
friday
saturday
sunday
every
everyday