WIP: Refactor - Githubissues

MycroftAI / skill-alarm

Mycroft AI official Alarm Skill - Set single and recurring alarms, with a choice of alarm sounds

https://mycroft.ai/skills

Apache License 2.0

14 stars 32 forks source link

WIP: Refactor #108

Closed krisgesling closed 3 years ago

krisgesling commented 3 years ago

Description

In preparation for bug fixes, this is a clean up of the code base with no functional changes.

So far:

Functions have been made pure and extracted where possible.
Basic unit tests have been added for all extracted functions.

To do:

clean up code
create Beeper/Notifier and Alarm classes
break down larger intent handlers and extract common code

Type of PR

[x] Refactor of code (without functional changes)
[x] Test improvements

Testing

Integration tests pass Everything continues to work as previously.

To run unit tests:

source ~/mycroft-core/.venv/bin/activate
cd /path/to/skill/util
pytest

chrisveilleux commented 3 years ago

I don't like using util as the package name for the abstracted code. To me, util implies functionality that has nothing to do with business logic. Examples include logging, file/directory handling, wrappers around third party tools, etc. I would like to see this directory named either skill or lib or source. I also want to see the public API into this package defined in __init__.py. I want this to become a common practice at Mycroft.

For the tests, we already have a test directory for the behave tests. Unit tests should be included there. I don't like having two places for test code.

chrisveilleux commented 3 years ago

We need to stop using one letter variable names too.

krisgesling commented 3 years ago

k ;)