Allow "everyday"

[burnsfisher]

I don't plan to fix the other issue I posted :-) BTW, I also don't speak German. You'd better have a look at what google translate put for 'everyday'

[ChristopherRogers1991]

In addition to the other comments, could you add your username to the list of contributors in the README?

[ChristopherRogers1991]

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.

[burnsfisher]

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.

[ChristopherRogers1991]

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.

[ChristopherRogers1991]

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)

[burnsfisher]

All good points and ideas. I'll see what I can do tomorrow. Thanks!

…

On Fri, Jan 14, 2022 at 7:07 PM ChristopherRogers1991 < ***@***.***> wrote: ***@***.**** commented on this pull request. In addition to the other comments, could you add your username to the list of contributors in the README? ------------------------------ In __init__.py <#19 (comment)> : > @@ -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'] 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 in any are present. ------------------------------ In vocab/de-de/DaysOfWeek.voc <#19 (comment)> : > @@ -5,3 +5,4 @@ Donnerstag Freitag Samstag Sonntag +Jedentag I don't speak German either - perhaps @gras64 <https://github.com/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) — Reply to this email directly, view it on GitHub <#19 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AASNGPHTWJDE3XEBLMWQ4JLUWC3CLANCNFSM5L74JEKQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you authored the thread.Message ID: <ChristopherRogers1991/mycroft_routine_skill/pull/19/review/853468899@ github.com>

[ChristopherRogers1991]

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.

[burnsfisher]

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!

[ChristopherRogers1991]

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).

[ChristopherRogers1991]

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.

[burnsfisher]

Thanks for again reviewing. Let me respond a couple ways:

1. I've done pull requests with another friend, but always only one at a time. In this case, I did a pull request, you reviewed and made suggested, I changed things, but I could not make another pull request. Ok, I see that when a pull request is open, that is part of what gets merged. However, it appears that in your first comment from about 1800 UTC 16-Jan, the code you copied does not have my change in it. In fact the latest code is like this:

    def _get_days(self):
        days_to_run = []
        days_from_user = self.get_response('which.days')
        if not days_from_user:
            return
        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:
                if (i >= 7) and (len(days_to_run)==0) :
                    days_to_run = ['0','1','2','3','4','5','6']
                elif i < 7:
                    days_to_run.append(str(i))
        return ','.join(days_to_run)

So that allows any number of equivalences to everyday. Certainly I agree that does not solve 'weekends' and 'weekdays' etc but that was not a goal either for this change or for your original. I believe this works if there are different numbers of 'extra' values for everyday in other languages since it is based on len(self._days_of_week).

I do totally understand what you are saying about a file for each day and for each group name (weekend, weekday, every day, etc) and it makes total sense. However, given my limited time, I'll suggest if you are willing to accept "every" this way and place the rest as an issue.

I have only glanced at your comments about 'try'. I'll look again and comment more and/or make changes for that.

[burnsfisher]

Hi, I agree that there are some issues -- i.e. that if you get an exception in cron you return with no notification. However, my question really is this: If there is something wrong, even if you don't know what it is, do you really WANT to update the schedule file? Something is broken. Of course before I put the try/except case in it STILL would not have written the file because it was an uncaught exception. Let me see what I can do--I look forward to your comments on a possible improvement. Thanks...

…

On Sun, Jan 16, 2022 at 2:27 PM ChristopherRogers1991 < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In __init__.py <#19 (comment)> : > @@ -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: 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). — Reply to this email directly, view it on GitHub <#19 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AASNGPEUJOYR3CELHCSCHZDUWMLZ3ANCNFSM5L74JEKQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you authored the thread.Message ID: <ChristopherRogers1991/mycroft_routine_skill/pull/19/review/853711945@ github.com>

[burnsfisher]

Ok, I reproduced that problem with the code before I changed it. Suppose you say

schedule test #(Test is a real routine)
mumble day # Any invalid day
6:01 p.m.

The first result is an exception in the call to from_crontab /_schedule_routine/_add_routine_schedule complaining about the wrong number of fields (got 4 expected 5). That's because _get_days returns a null list. But it has also edited routines2.json to be the following:

{
"name": "testing",
"tasks": [
"show me the time"
],
"schedule": "1 6 * * ",
"enabled": true
}
which causes a similar error in the init routine of the skill when it tries to load and call from_crontab, so it will never load again.
So just fyi, that is the kind of error I was trying to fix, in addition to the spoken except error when time is bad.

I'll see about improving those.

[burnsfisher]

Ok, this is my proposal. I did not change to multiple day voc files. As I said, that's not the current goal, and is easy enough to do if we wanted more day aliases.

I did fix the time and date fixes as I proposed before. One thing I might have done but did not was make a different dialog for not speaking a day or time soon enough vs speaking an incorrect time. I can do that, but the generic dialog for a day or time error seems like a sufficient improvement over a generic exception and corruption of the schedule file.

It will also allow and just ignore a bogus day in the list with good ones, e.g. Monday Tuesday Horrible Thursday. Again, this is how it worked before. Perfect is the enemy of the better and all that.

[burnsfisher]

Christopher, I appreciate your previous comments. The code is better for them. I'm wondering if you have any more before you are willing to merge this? At some point I can do the other things you suggested, but those seemed like enhancements to me rather than changes to the basic fix I was making. Thoughts?