Fixed the flake8 errors, Travis CI should be happy now (I hope)

Here's a screenshot, I have USE="vlc dbus zeroconf". I do not use mpv or the Spotify Web player on my system.

Hi, thanks for the PR!

A few comments/questions:

• The is_installed method should be somwhere else to avoid code duplication. Either at vidify/__init__, or in a new file. I was thinking of creating a new base data structure for the APIData and PlayerData because after all they're almost the same (specially now that players also have a platform field).
• This new is_installed should take an *args parameter rather than name: str, because some APIs or Players may depend on multiple libraries.
• Did you find any issues with is_installed? Back when you added it to setup.py I remember we had some issues with PySide2, because it doesn't include a wheel and get_distribution only checks for wheels. It might work now but we have to make sure it's reliable enough.
• This doesn't check external dependencies either. But I'm not sure if they should be taken into account anyway. For example, the VLC players needs both the player itself, and python-vlc, but this only checks the latter.
• Not installed APIs and Players should have a text with "Not installed", rather than "Unavailable". The former indicates that it can be installed, it's just not in your system. The second makes it clear that you can't even install it. Or at least something similar to differentiate the cases. Maybe unavailable APIs shouldn't even appear at all.

Hi, thanks for the PR!

A few comments/questions:

* The `is_installed` method should be somwhere else to avoid code duplication. Either at `vidify/__init__`, or in a new file. I was thinking of creating a new base data structure for the `APIData` and `PlayerData` because after all they're almost the same (specially now that players also have a platform field).

The is_installed can definitely go elsewhere, good idea. The APIs still have some extra optional arguments (connect_msg, gui_init_fn, event_loop_interval), and the Players still take the non-optional flags. So integrating these into one single file would require a bit more work, I'd have to think about how to do that a bit more.

* This new `is_installed` should take an `*args` parameter rather than `name: str`, because some APIs or Players may depend on multiple libraries.

Aaah yes, good idea. This shouldn't be to difficult to fix I think, will look into it.

* Did you find any issues with `is_installed`? Back when you added it to `setup.py` I remember we had some issues with PySide2, because it doesn't include a wheel and `get_distribution` only checks for wheels. It might work now but we have to make sure it's reliable enough.

Not so far, but I haven't tested swspotify as I am not on windows. I had issues with PySide2 because their setup.py doesn't seem to provide proper metadata (e.g install-requires, packages=, version=). At least that is what I think was the problem. This function checks if their is a package(-version) matching package=(, version=) (I think). So this should work for any package with proper metadata, which is every package with a proper setup.py.

In fact, this should work for every package that is listed as install-requires= in Vidify's setup.py. Because the function get_distribution is the very same function that is executed at the start of every python program. Starting any python program, makes the interpreter check if the requirements are installed as specified in .egg-info (I think), if not you get this Distribution not Found error.

If Vidify is installed with setup.py install to the system, launching it without e.g. tekore gives an error like "tekore-x.y is not installed and is required by Vidify". That is also how I found out about the Pyside2 issue. PySide2 was listed in setup.py and starting Vidify was giving me the Distribution not Found error even though PySide2 was installed. So long story short, the is_installed check works for any package that can also be added to install-requires= without problems.

* This doesn't check external dependencies either. But I'm not sure if they should be taken into account anyway. For example, the VLC players needs both the player itself, and `python-vlc`, but this only checks the latter.

IMO, it is up to the distribution's package manager to deal with the non-python dependencies of e.g. python-vlc. If python-vlc is installed without vlc actually being installed, then that is a problem with the packaging of python-vlc in that distribution. I think we can safely assume that if python-vlc is present that vlc is also present on the system (On Gentoo at least dev-python/python-vlc depends on media-video/vlc)

* Not installed APIs and Players should have a text with "Not installed", rather than "Unavailable". The former indicates that it _can_ be installed, it's just not in your system. The second makes it clear that you can't even install it. Or at least something similar to differentiate the cases. Maybe unavailable APIs shouldn't even appear at all.

Will look into this as well, shouldn't be too hard to get it display an alternative text.

The is_installed can definitely go elsewhere, good idea. The APIs still have some extra optional arguments (connect_msg, gui_init_fn, event_loop_interval), and the Players still take the non-optional flags. So integrating these into one single file would require a bit more work, I'd have to think about how to do that a bit more.

My idea is using inheritance, having a base of which APIData and PlayerData inherit from, avoiding hacks to have them both in the same class. The thing is that I don't know if it's possible with custom Enums, because we are overriding __new__ to add parameters. Perhaps with *args or something like that. I have to investigate about what data structures are best for this.

Not so far, but I haven't tested swspotify as I am not on windows.

I can do that. I'll make sure it doesn't break Windows support.

In fact, this should work for every package that is listed as install-requires= in Vidify's setup.py. Because the function get_distribution is the very same function that is executed at the start of every python program. Starting any python program, makes the interpreter check if the requirements are installed as specified in .egg-info (I think), if not you get this Distribution not Found error.

Ah nice, I didn't know that. I'll just make sure it works well with other packages and leave it be then.

IMO, it is up to the distribution's package manager to deal with the non-python dependencies of e.g. python-vlc. If python-vlc is installed without vlc actually being installed, then that is a problem with the packaging of python-vlc in that distribution. I think we can safely assume that if python-vlc is present that vlc is also present on the system (On Gentoo at least dev-python/python-vlc depends on media-video/vlc)

Yes, that's what I was thinking. The only way to do this would be with a try: except: anyway, and that's a bit nasty.

Will look into this as well, shouldn't be too hard to get it display an alternative text.

Anyway I really like the idea of not even showing APIs and Players that aren't available on the OS. It's probably confusing for some users, and it's not necessary.

My idea is using inheritance, having a base of which APIData and PlayerData inherit from, avoiding hacks to have them both in the same class. The thing is that I don't know if it's possible with custom Enums, because we are overriding new to add parameters. Perhaps with *args or something like that. I have to investigate about what data structures are best for this.

I've got no clue about this to be honest, but I think I addressed all your other comments. is_installed is now in vidify/__init__ and it accepts any number of string arguments, it returns False if one of those arguments is an unavailable package and otherwise returns True. The "Unavailable" text is now "Not Installed", and API/Players that are not available on this platform are not shown at all:

I think we should adjust setup.py as well, because even though we can now handle correctly cases where APIs/Players are not installed. Vidify will still fail to start if e.g. tekore is not installed. Because all the APIs/Players are still listed as required in the .egg-info file, since they are still in install-requires=. Maybe we should put most of the API's/Players in extras-require instead, what do you think?

Well looks like I broke something with mpv 😢

Traceback (most recent call last):

  File "/home/travis/build/vidify/vidify/tests/test_api_and_player_data.py", line 58, in test_imports_and_class_names_in_modules

    initialize_player(player.name, config)

  File "/home/travis/build/vidify/vidify/vidify/player/__init__.py", line 89, in initialize_player

    mod = importlib.import_module(player.module)

  File "/home/travis/virtualenv/python3.6.7/lib/python3.6/importlib/__init__.py", line 126, in import_module

    return _bootstrap._gcd_import(name[level:], package, level)

  File "<frozen importlib._bootstrap>", line 994, in _gcd_import

  File "<frozen importlib._bootstrap>", line 971, in _find_and_load

  File "<frozen importlib._bootstrap>", line 955, in _find_and_load_unlocked

  File "<frozen importlib._bootstrap>", line 665, in _load_unlocked

  File "<frozen importlib._bootstrap_external>", line 678, in exec_module

  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed

  File "/home/travis/build/vidify/vidify/vidify/player/mpv.py", line 15, in <module>

    from mpv import MPV

  File "/home/travis/virtualenv/python3.6.7/lib/python3.6/site-packages/mpv.py", line 563, in <module>

    _handle_func('mpv_render_context_create',               [MpvRenderCtxHandle, MpvHandle, POINTER(MpvRenderParam)],   c_int, ec_errcheck,     ctx=None)

  File "/home/travis/virtualenv/python3.6.7/lib/python3.6/site-packages/mpv.py", line 474, in _handle_func

    func = getattr(backend, name)

  File "/opt/python/3.6.7/lib/python3.6/ctypes/__init__.py", line 361, in __getattr__

    func = self.__getitem__(name)

  File "/opt/python/3.6.7/lib/python3.6/ctypes/__init__.py", line 366, in __getitem__

    func = self._FuncPtr((name_or_ordinal, self))

AttributeError: /usr/lib/x86_64-linux-gnu/libmpv.so.1: undefined symbol: mpv_render_context_create

Not sure why that is all of a sudden not working, I don't think I changed anything related specifically to mpv in the last push

I don't get this error running python -m unittest on my system (after installing tekore and python-mpv). Maybe it is something with the Travis CI bot? (Maybe it is using a different version of mpv/python-mpv then it was yesterday???)

I think we should adjust setup.py as well, because even though we can now handle correctly cases where APIs/Players are not installed. Vidify will still fail to start if e.g. tekore is not installed. Because all the APIs/Players are still listed as required in the .egg-info file, since they are still in install-requires=. Maybe we should put most of the API's/Players in extras-require instead, what do you think?

I've thought of moving dependencies like dark mode or tekore to extras-require but I came to the conclusion that it's most likely not worth it. Even though Vidify is supposed to be modular, it would be a pain to install it IMO. Sane defaults are important in software, too, and even though we both prefer to customize everything (see Arch and Gentoo), most people don't.

I think it's better that the package has 5 more dependencies by default than that the user has to spend 10 minutes reading documentation and what packages they have to install. I would leave as it is.

On the other side, different installation methods can solve this. For example, the binary download is probably the most user-friendly, and already includes all extra requirements. Thus, advanced users like those using Arch, Gentoo, or pip, would be able to customize the installation.

I don't get this error running python -m unittest on my system (after installing tekore and python-mpv). Maybe it is something with the Travis CI bot? (Maybe it is using a different version of mpv/python-mpv then it was yesterday???)

You're right... I've restarted the tests on master and the same error happens:

https://travis-ci.com/github/vidify/vidify/jobs/313324477#L1502

A new version of python-vlc was released 8 hours ago, and apparently this is the line that breaks. Have you tried with v0.4.5?

On the other side, different installation methods can solve this. For example, the binary download is probably the most user-friendly, and already includes all extra requirements. Thus, advanced users like those using Arch, Gentoo, or pip, would be able to customize the installation.

Well it doesn't really matter because I can just sed out the lines in setup.py in the ebuild to allow Vidify to work without those dependencies. It's only relevant for people who actually use pip (or do python setup.py install), and to be honest I have no idea how many people actually use pip. I know Gentoo users don't, because pip and portage are not compatible at all (Portage is written in Python, and using Pip usually ends up breaking Portage, which is why Gentoo only allows pip with the --user option). Ideally setup.py would support more advanced dependency resolution, like the or operator, but lacking that we should perhaps just keep it like this then.

A new version of python-vlc was released 8 hours ago, and apparently this is the line that breaks. Have you tried with v0.4.5?

I was using 0.4.5, just version bumped to 0.4.6, same results. :(

I was working today on the Dockerfile to start using GitHub actions instead of Travis CI, and for some reason this bug doesn't happen anymore on the new CI...

https://github.com/vidify/vidify/runs/562797112?check_suite_focus=true

Do you still get the error? I can't reproduce it on my machine.

Do you still get the error? I can't reproduce it on my machine.

No, I only get this error in this PR, not when running the tests locally on my system.

I was working today on the Dockerfile to start using GitHub actions instead of Travis CI, and for some reason this bug doesn't happen anymore on the new CI...

How strange 😕 I wonder what is causing this to occur specifically in Travis CI. Do the GitHub actions fetch and install (python-)mpv in the same way that Travis does it?

Travis wasn't using a docker container back then. Only the Travis container itself, which was Ubuntu Xenial 16.04. Now GitHub Actions uses Debian Buster 10, so it might have been that... Anway it's most likely something unrelated to Vidify. Can you do a rebase or a commit so that the new CI does a run?

Can you do a rebase or a commit so that the new CI does a run?

It's working now 😄

Alright! I'm merging the PR, thanks. I'll also (try to) do a small refactor to have a base Data class to avoid code duplication when declaring APIData and PlayerData, and to only have one load_apis method instead of two, now called load_data. Then, I'll release 2.2.3 :)

Awesome, thanks!