Fix logic in determining whether to add PySide2 or PyQt5 to deps

[AndrewAmmerlaan]

I just realized that my logic in determining whether or not to append PyQt5 to dependencies does not correctly handle the case where PyQt5 AND PySide2 are installed but not PyQtWebengine: (False or True) and False == False where it should be true.

if ((get_dist('PyQt5') is None or get_dist('PyQtWebEngine') is None)
        and get_dist('PySide2') is None):
    install_deps.append('PyQt5')
    install_deps.append('PyQtWebEngine')

Instead I propose this logic:

if get_dist('PySide2') is not None:
    install_deps.append('PySide2')
else:
    install_deps.append('PyQt5')
    install_deps.append('PyQtWebEngine')

Now, if PySide2 is not None (=installed) it is appended to the dependencies. If not, then it reverts to append PyQt5 and PyQtWebengine. I think this (simpler) logic should cover all cases. What do you think, is this better?

I also changed the ** to * because it does the same. In theory the double ** should match recursively: https://docs.python.org/3/library/glob.html but apparently it doesn't work like that in package_data because otherwise 'gui/res/** would already be enough to also install the subdirectories (which it is not, without the other two lines it doesn't install the subdirectories).

[marioortizmanero]

That does sound like a simpler solution, thanks for the PR. Maybe it would even be simpler if the function was is_installed() and it returned a boolean rather than the installation dir (which we don't need). But that's just an idea. Also, please add a small comment indicating what it does.

About the ** glob, does the glob module not work for this case? I saw you used it in your example in your last PR and it looked nice.

[AndrewAmmerlaan]

Maybe it would even be simpler if the function was is_installed() and it returned a boolean rather than the installation dir (which we don't need). But that's just an idea. Also, please add a small comment indicating what it does.

Fixed, I also changed it to this:

if is_installed('PySide2') is True and is_installed('PyQt5') is False:
    install_deps.append('PySide2')
# If PySide2 is not installed, or if both PyQt5 and PySide2 are installed
# Use QtPy's default: PyQt5
else:
    install_deps.append('PyQt5')
    install_deps.append('PyQtWebEngine')

Because if both PySide2 and PyQt5 are installed vidify should depend on PyQt5 because it is QtPy's default. This way PySide2 is only used if it is the only Qt implementation present.

About the ** glob, does the glob module not work for this case? I saw you used it in your example in your last PR and it looked nice.

I tried this package_data={'': glob('vidify/gui/res/**', recursive=True)} but it doesn't work, I think that is because of this issue: pypa/setuptools#1806

[marioortizmanero]

You should change the boolean logic to the following:

if is_installed('PySide2') and not is_installed('PyQt5'):
    install_deps.append('PySide2')

Easier to read and more natural. Apart from that, great job :) I'll keep track of that PyPa issue for the future.

[AndrewAmmerlaan]

You should change the boolean logic to the following:

if is_installed('PySide2') and not is_installed('PyQt5'):
    install_deps.append('PySide2')

Easier to read and more natural. Apart from that, great job :) I'll keep track of that PyPa issue for the future.

Fixed

[marioortizmanero]

Also, consider using type hinting for is_installed:

def is_installed(pkgname: str) -> bool:
    try:
        get_distribution(pkgname)
        return True
    except DistributionNotFound:
        return False

[AndrewAmmerlaan]

Also, consider using type hinting for is_installed:

def is_installed(pkgname: str) -> bool:
    try:
        get_distribution(pkgname)
        return True
    except DistributionNotFound:
        return False

Fixed

[marioortizmanero]

Thanks! I'm merging then.