MycroftAI / skill-wiki

Query Wikipedia articles
https://mycroft.ai/skills
Apache License 2.0
18 stars 33 forks source link

Refactor: splitout Lookup, fix failing "science" testcase #55

Closed forslund closed 3 years ago

forslund commented 3 years ago

Description

This started as a fix for the failing lookup of "science" but mutated into some sort of refactoring since the fix required some additional changes and it sort of snowballed. It does however fix the failing test case.

Things done: Split the response handling from the lookup code: This allows doing the lookup both with auto-suggest and without it. If a Disambiguation result and a Page Match result are both received the page match is used. To keep / improve the speed auto-suggest and non-autosuggest versions are run in parallell.

Made the skill more stateless The article information is no longer passed via the self.result member, instead it's passed through adapt context. This should be better in the cases where multiple clients are connected.

Minor refactoring The repeated gui code was moved into a reusable method. Comments and docstrings were improved. the _lookup method was in the end moved out of the Skill class since it barely uses the class methods.

Things to note The PageDisambiguation class is a bit thin, was mainly created to easily be able to use the response type of the lookup to see what type of response was received. I think it's existence is motivated since it improves readability.

Type of PR

Testing

How can someone reviewing this PR test that it is working properly? Is there appropriate test coverage for this change?

Documentation

Does documentation for this already exist? Are there docstrings on all the public methods you added or modified? Have these been updated?

CLA

To protect you, the project, and those who choose to use Mycroft technologies in systems they build, we ask all contributors to sign a Contributor License Agreement.

This agreement clarifies that you are granting a license to the Mycroft Project to freely use your work. Additionally, it establishes that you retain the ownership of your contributed code and intellectual property. As the owner, you are free to use your code in other work, obtain patents, or do anything else you choose with it.

If you haven't already signed the agreement and been added to our public Contributors repo then please head to https://mycroft.ai/cla to initiate the signing process.

krisgesling commented 3 years ago

oops...

forslund commented 3 years ago

Thanks for the review, I've updated according to your suggestions. Now time to head into the office :)

forslund commented 3 years ago

I pushed an update to handle if the auto-suggest version of the lookup returned no answer. This should make the skill correctly find the elon musk article.

krisgesling commented 3 years ago

Working great, thanks Ake!!