forslund / mopidy_skill

A Mycroft skill for controlling the mopidy media player
GNU General Public License v3.0
15 stars 14 forks source link

Improve handling of GMusic album names. #28

Closed ProjectMoon closed 7 months ago

ProjectMoon commented 4 years ago

This fixes the skill crashing when encountering weird album names in GMusic responses from Mopidy.

Open items:

  1. Unit tests. Do all tests go in test/__init__.py or should I make a separate test file? Also, what unit test framework?
  2. There is an opportunity to further improve the handling of album names here. What if an artist has a - in their name? That will produce weird results. I imagine it would be very uncommon, but still possible. A suggestion would be to probably take the last entry in the split array, rather than always the second (included in this PR).
forslund commented 4 years ago

1. The __init__.py in the test folder is intended for setting the test environment for the intent test so I suggest a separate file.

I prefer the python builtin unittest framework for simplicity.

2. Indeed it's not fool proof, as you suggest indexing the last with [-1] would probably be an easy improvement. We should definitely look if that information can be fetched more reliably at a later time.

forslund commented 4 years ago

Thanks for fixing this I'll do a quick test run of it tomorrow and then merge it if nothing terrible (ominous thunder) happens during the test run.

ProjectMoon commented 4 years ago

I want to add unit tests first. Also I need to double check something with the -1 index.

ProjectMoon commented 4 years ago

Yes, need to add an if check. Splitting on a non existent token and grabbing the -1 index will give you the original string.

ProjectMoon commented 4 years ago

So, the code relies on the mycroft module, which does not seem to be available to import from the Mycroft Skills Kit. Am I doing something wrong? The documentation says it should be in mycroft.skills. But adding the msk package doesn't have a mycroft module.

This is blocking me from creating the tests.

forslund commented 4 years ago

mycroft-core sets up and runs in it's own virtualenv. to access the mycroft package you need to activate that venv. in the mycroft-core folder there is a venv-activate.sh file. Running source venv-activate.sh from the mycroft-core folder should be enough to make it available when running the unittest.

Let me know if something's unclear or if this doesn't work.

ProjectMoon commented 4 years ago

Is there a better way to get the unit test working in isolation, without requiring a full installation of Mycroft locally?

forslund commented 4 years ago

If you want to test the mopidypost file alone, this is what I could get going:

then you should be able to run pytest test_mopidypost.py to test the mopidypost module.

Edit: Needed to do some submodule rejiggering to get pytest to leave mycroft entirely out of things.