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'

[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 https://github.com/ChristopherRogers1991/mycroft_routine_skill/pull/19#discussion_r785233149 :

@@ -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 https://github.com/ChristopherRogers1991/mycroft_routine_skill/pull/19#discussion_r785235906 :

@@ -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 https://github.com/ChristopherRogers1991/mycroft_routine_skill/pull/19#pullrequestreview-853468899, 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>

[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 https://github.com/ChristopherRogers1991/mycroft_routine_skill/pull/19#discussion_r785484623 :

@@ -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 https://github.com/ChristopherRogers1991/mycroft_routine_skill/pull/19#discussion_r785484623, 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?