mycroft-skills icon indicating copy to clipboard operation
mycroft-skills copied to clipboard

Published 20 hours ago • verified publisher icon MycroftAI

lecture-skill

Open Habanossi opened this issue 5 years ago • 5 comments

Name of your skill: Lectures-skill

Description:

When asked what kind of lecture there is today, mycroft answers with the right subject. this works as a teaching example for home assistants.

Checklist:

  • [x] Used Meta Editor to generate the skill README
  • [x] Skill has been tested and works
  • [x] Read and verified approval process - https://mycroft.ai/documentation/skills/skills-acceptance-process (Can require a community member vouching for skill)
  • [x] .gitmodules has been updated with the following:
+[submodule "NAME OF YOUR SKILL"]
+	path = name-of-your-skill
+	url = URL.FOR.YOUR.SKILL.git

Habanossi avatar May 09 '19 08:05 Habanossi

Hi Habanossi,

Welcome to the Community!

I noticed you had some trouble with submodules. We've recently updated our Mycroft Skills Kit to automate this process for you, but I haven't yet updated the submission documentation. So assuming you have Mycroft installed you can now run: mycroft-msk submit /opt/mycroft/skills/your-skill-directory This will prompt you for your Github username and password then add the submodule and create a PR.

The 'needs validation' tag I've added is just a flag to our Skills Testing Team to come and review this Skill in line with our Skills Acceptence Process. They might suggestion amendments, but once approved we'll merge it and the Skill would then be available in the Mycroft Marketplace for anyone to easily install.

If you have any questions, you can reply here or join us in the ~Skills Channel on Chat

krisgesling avatar May 10 '19 01:05 krisgesling

I will start with the code review.

ricargoes avatar May 12 '19 11:05 ricargoes

I have limited myself to review the code however I do not get what is this skill for. Maybe there is something I have missed, but it seems like the functionality provided is very specific to the authors situation.

In any case, if there is something that needs clarification or something I can help with, just let me now.

Meta

  • Platform: Linux
  • Mycroft-core version: 19.2.6
  • Who: @ricargoes / @ricargoes
  • Datestamp: 2019-05-12_14:38:23_CEST
  • Language and dialect of tester: Spanish from Spain (review made in English)

0. Automated tests

Are all automated tests passing?

  • [ ] Skill tester - Jenkins
  • [x] Continuous Integration - Travis-CI

1. Code Review - secure and stable

  • [ ] Code Quality
    Can you understand what the code is doing? Is there inline documentation? Do you have any concerns about this code running on your machine? Are there any performance issues such as nested or infinite loops? Do you have significant concerns about the overall code quality?
    NOTE: We do not enforce PEP8 Checks on Skills

MAJOR ISSUES: (Fixing is required)

  • In lines 27, 31, 60 and 63 absolute paths to the skill folder are given. There is no guarantee the skills folder will be in /opt/mycroft/skills, for starters it will not be in any OS other than linux. Mycroft libraries provide enough tools to abstract settings and manipulation of files from specific machines.

MINOR ISSUES: (Recommended to fix but not required)

  • In line 42, a manual parsing is done to get a datetime object. It would be more efficient and easier to use the datetime built-in string parser: datetime.strptime
  • In line 45 when a condition is met two things are meant to happen: 1) we record info and 2) we want to break the loop. In the code a boolean is use for both purposes, it is set to true and then it is evaluated in the while condition. A good practice would be to split responsabilities and introduce a break in the if block. It would improve readability, efficiency and robustness.
  • Using wait_while_speaking would be recommended to avoid unexpected behavior from mycroft.
  • In line 60-63 there is no need to open the file twice, just open it once with rw permissions.
  • [ ] Error Handling
    Are there any specific checks we make for error handling or graceful degradation?

No error handling whatsoever. It would be recommended to control some of the errors that may arise, like not being able to read date.txt and settings.txt. In addition, python strongly recommends to always open documents inside a with block.

  • [x] Libraries
    Does the Skill include the correct libraries? Does it use too many libraries or dependencies?

No special libraries are needed.

  • [x] Required Dependencies
    Check requirements.txt and requirements.sh - are the required dependencies listed? If requirements.sh is used, is some form of conditional processing done to match against multiple distros? Often Skill Authors will add requirements.txt using only an “library=1.x.x” instead of “library >=1.x.x”. Check to make sure that there is an equal or greater than in the requirements to help future-proof the Skill, unless a specific version is needed.

No special libraries are needed.

  • [ ] Settings
    Is the settingsmeta.json file well laid out? If settings are not used, has the default file been deleted? If it is the default file, the first setting section will be called "Options << Name of section". >

The settings file is left untouched, with the placeholders and all.

  • [x] Integration Tests
    Does the skill include sufficient integration tests, included in the test folder?

There are a couple of integrations tests.

  • [ ] Other Files
    Are there any other files included that are unnecessary or you are unsure of their function?

All files included are used at some point in the code. However, there seems to be a overlapping between the functionality of settingsmeta.json, settings.txt and dates.txt.

Actions Required:

  • Move the functionality of settings.txt and dates.txt to the standard mycroft settings settingmeta.json. This would also solve the issue with absolute paths and with the error handling of open files.

ricargoes avatar May 12 '19 12:05 ricargoes

Meta

  • Who: @lb803 / @lb803
  • Datestamp: 2019-05-13_20:59:18_BST
  • Language and dialect of tester: English (second language)

3. Functional Review - intuitive and expected

  • [ ] Installation

Check that the Skill installs using voice commands. Mycroft will get the user to confirm which Skill should be installed if there is ambiguity in Skill names - such as duplicate names. If possible, name the Skill so that there is minimal duplication and/or conflict. You should also verify that the Skill name can be verbally pronounced by speaking the Skill name into the Mycroft command line several times, and reading the resulting transcriptions. Suggest alternative Skill names if it is difficult to verbally pronounce the Skill name. Please provide confirmation that the Skill was successfully installed and by what means (voice or mycroft-msm install), as well as what utterance was detected when invoking the install voice command.

Install method: cloned git repo, voice installation not working yet.

  • [ ] Settings
    If Skill includes a settingsmeta.json file - are the settings well laid out? Does the placeholder text make sense? This can also be checked on home.mycroft.ai/#/skill

The skill has a settingsmeta.json placeholder file, and it is not used.

  • [x] Dialog

Check the dialog directory to ensure that from a voice user interface perspective the dialogs read well. Alway play every dialog phrase on the command line by doing say so that you can check how the dialog is spoken. It is a good idea to run the dialog phrases through mimic.

Sometimes the dialog files will need a small tweak such as a space between words, or extra vowel sounds, to sound realistic. Sometimes words don't render as expected and alternative wording should be used. Some of the tricks you might need to use include separating words by a space - such as sub sonic instead of subsonic, or broad cast instead of broadcast.

Dialogs work ok.

Skill Functions

For each function of the Skill add a new checkbox with the utterance used to invoke the functionality. Confirm the output and behaviour of each. If any setup is required to perform these tests please indicate this directly before the test is described.

  • [x] "What is the subject of the lecture" (with lecture set)

The subject of the lecture is privacy

  • [x] "What is the subject of the lecture" (with lecture not set)

No lecture today

  • [x] "Change the lecture settings"

Default settings are set

  • [x] "What theme does the lecture have"

The subject of the lecture is privacy

Actions Required

A short list of any Actions Required. It is also great to provide a short statement of your impressions from using the Skill.

  • The skill might not work after the installation. Config file paths are hardcoded (lines 27, 31, 60, 63 in the __init__.py file) and, unfortunately, they didn't match with my installation. I suggest making python figure this out via os.path.join(os.path.dirname(__filename__), 'your_config_file.txt') python doc. This has to be fixed before market acceptance.
  • The skill is hard to set up. To get the expected results, the user has to manually edit text files and follow a very rigid procedure (documented in the README.md file) to make Mycroft match dates of the lectures with subjects. I suggest to devise an easier way to set up the skill, as not all Mycroft users might feel comfortable editing text files or just might expect a more friendly user experience.
  • I don't like the idea of modifying dialog files to (individually) customize Mycroft responses. More simply, you could have a generic dialog and then pass some data to the speak_dialog method to specialize Mycroft's response.

lb803 avatar May 13 '19 21:05 lb803

Meta

  • Who: @GPVM / @gpvm
  • Date and time: 18 May 2019
  • Language and dialect of tester: English (Canada)

2. Information Review - accurate and understandable

This review checks the README for completeness - does it follow the README template and include all the relevant sections such as Intents, known issues, dependencies and so on?

  • [ ] Icon
    Does the Skill have an appropriate icon that is either included in the repo or linked from an appropriate place (eg raw.githack.com not privateicons.com or rawgit.com)?

icon is missing.

  • [ ] Description
    Are the title, short description, and long description (under 'About') clear and concise? Do they provide enough information to understand what the Skill does? Does the title have the word "skill" included? This is strongly discouraged.

I am struggling with this Skill. The description contains a lot of detailed instructions which are not very clear to me. Also, the Skill seems very specific to a single user's needs and not a community-wide skill. Nevertheless, I have proposed some edits to the Readme.md file through a new issue for the repo.

  • [x] Examples
    Do the examples give you a clear understanding of how you can use the Skill? Is there a single example per dot-point?

yes

  • [ ] Supported Devices
    If relevant, are the supported devices listed? An example might be a Skill that requires the screen of the Mark II. If the section is not present, all devices are considered supported.

no constraints listed

  • [ ] Categories
    Is there at least one category listed? At least one category must be selected for the Skill to be displayed correctly in the Mycroft Marketplace.
    Is the bolded category the most appropriate for this Skill? The bold category is considered the primary category and will be used for display in the Marketplace, all others are secondary and used for search results.

no categories listed

  • [ ] Tags
    Are listed tags appropriate for this Skill?

no tags listed

  • [ ] License
    Is an appropriate LICENSE file used in the repo - such as Apache-2.0 or GPL-3.0?

no license is listed

GPVM avatar May 19 '19 03:05 GPVM