skill-weather icon indicating copy to clipboard operation
skill-weather copied to clipboard

fix condition/.. requests with location information

Open emphasize opened this issue 4 years ago • 4 comments
trafficstars

To reproduce:

issue a fog / sunrise report for a specific location ("is it foggy in New York")

Problems:

  • Several intents (condition/storm/rain/...) have .optionally("Location") in their constructor while others go with .optionally("location"). This resulting in the location getting ignored.

  • If you issue a sunrise request at a specific location now = now_local(tz=self.intent_data.geolocation["timezone"]) throws an exception

Traceback (most recent call last):
  File "/home/sweng/mycroft-core/mycroft/skills/mycroft_skill/event_container.py", line 73, in wrapper
    handler(message)
  File "/opt/mycroft/skills/skill-weather.emphasize/__init__.py", line 565, in handle_sunset
    dialog.build_sunset_dialog()
  File "/opt/mycroft/skills/skill-weather.emphasize/skill/dialog.py", line 170, in build_sunset_dialog
    now = now_local(tz=self.intent_data.geolocation["timezone"])
  File "/home/sweng/mycroft-core/mycroft/util/time.py", line 68, in now_local
    return datetime.now(tz)
TypeError: tzinfo argument must be None or of a tzinfo subclass, not type 'str'
  • Typo in dialog string constructor for sunrise/sunset self.name += ".sunset.future" (should be "-sunset-future")

What this PR does

  • all location adapt intent construction strings are ~~lower case~~ capitalized
  • dialog.py imports get_tz_info from skill.util to convert the timezone string
  • Fixes the sunset/sunrise dialog construction

Type of PR

  • [x] Bugfix

  • [x] CLA

emphasize avatar Jul 31 '21 17:07 emphasize

I would think that this should be Location with capital L since the regex match group has the capitalization in the location.rx

There is something strange here since the vocab file for location (location.voc) has small letters so this won't be used in addition to the regex. If so it could maybe be removed?

forslund avatar Aug 02 '21 14:08 forslund

@forslund Then self.location = message.data.get("location") has to be changed too. #. Was about to change that (and this one worked on my test rig), as i saw that the constructor using both Loc../loc... Did't take regex into account.

Late changes > bad

emphasize avatar Aug 03 '21 10:08 emphasize

I wondered about the location.voc though as it would restrict the locations to choose from, basically leaving location None if not one of those is hit in the intent. I took it as a residue. Overlooking that it now would do exactly that.

Will revert to uppercase in the constructors and message key get.

emphasize avatar Aug 03 '21 11:08 emphasize

Hey emphasize, thanks heaps for all the detective work and fixes on these Skills!

I was just doing some testing on a blank Skill and with only a location.rx file, both the capital and lowercase work in the Adapt intent builder. But they should be consistent as the capitalization will definitely be a problem when accessing it on the message.data object. There are two more hanging around around lines 422 and 433.

The potential conflict of location.rx and location.voc is an interesting one too. So far in my testing the regex takes priority over the vocab.

Given this is @chrisveilleux's refactor I'll let him make the final call on when this is ready.

krisgesling avatar Aug 05 '21 04:08 krisgesling