search

MycroftAI / adapt

Adapt Intent Parser
Apache License 2.0
710 stars 154 forks source link

Fix storing regex as string when cleaned #135

Closed forslund closed 3 years ago

forslund commented 3 years ago

Removing regexp entity changed the type of the _regex_strings member to a list instead of a set. resolves #134

clusterfudge commented 3 years ago

@forslund can you provide a little more color on what this does and why it matters? The fact that the only test change asserts this internal variable is a set seems strange to me.

forslund commented 3 years ago

I can remove the test. It was mainly to prove to myself that the type remained set().

The issue is that the _regex_strings member is a set at code and .add() is called on it, the removal code was done using a list comprehension resulting in ._regex_strings member was a list after removal was called. a list doesn't have the .add() member so the issue in #134 happens.

emphasize commented 3 years ago

just realized that with the new weather skill i'm also not able to get a location on Message, so this might be problematic elsewhere. Or is this related?

j1nx commented 3 years ago

Tested the fix on OVOS with all latest and greatest packages (which had the same issue) and it solved the error.

forslund commented 3 years ago

@emphasize this would hinder it after reloading the skill but should work on first startup.

emphasize commented 3 years ago

yeah, tested it with the fix, but still no location on Message (what makes sense)

emphasize commented 3 years ago

The only meaningfull log i get is

2021-06-23 19:22:59.576 | DEBUG    | 21131 | mycroft.skills.skill_data:load_regex_from_file:64 | regex pre-munge: .*\b(bei|in) (?P<Location>(?!\bcelsius\b|\bfahrenheit\b) *.+)
2021-06-23 19:22:59.576 | DEBUG    | 21131 | mycroft.skills.skill_data:load_regex_from_file:66 | regex post-munge: .*\b(bei|in) (?P<skill_weather_emphasizeLocation>(?!\bcelsius\b|\bfahrenheit\b) *.+)
emphasize commented 3 years ago

Nevermind, Chris has written self.location = message.data.get("location") instead of self.location = message.data.get("Location")

forslund commented 3 years ago

Absolutely will add that.

forslund commented 3 years ago

Ok found another issue with this...since the tagger keeps a reference to the old regexp_entity this isn't actually working as intended...

forslund commented 3 years ago

Added a fix along with a test for that issue as well.

forslund commented 3 years ago

Version bumped 0.0.1 units :)