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.

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

fix condition/.. requests with location information #175

Open
wants to merge 3 commits into
base: 21.02
Choose a base branch
from

Conversation

emphasize
Copy link
Contributor

@emphasize emphasize commented Jul 31, 2021

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

  • Bugfix

  • CLA

@forslund
Copy link
Collaborator

forslund commented Aug 2, 2021

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?

@emphasize
Copy link
Contributor Author

emphasize commented Aug 3, 2021

@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
Copy link
Contributor Author

emphasize commented Aug 3, 2021

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.

skill/dialog.py Outdated
else:
self.name = ".sunset.past"
self.name = "-sunset-past"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found another one. This should be self.name += "-sunset-past"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@krisgesling
Copy link
Contributor

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.

__init__.py Outdated
@@ -430,7 +430,7 @@ def handle_is_it_foggy(self, message: Message):
self._report_weather_condition(message, "fog")

@intent_handler(
IntentBuilder("").require("ConfirmQuery").require("Rain").optionally("location")
IntentBuilder("").require("ConfirmQuery").require("Rain").optionally("Location")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is capitalized

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah it looks like this got changed in a recent commit from @chrisveilleux that's not yet on your fork.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants