Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PR: Support mypy #217

Closed
wants to merge 14 commits into from
1 change: 1 addition & 0 deletions MANIFEST.in
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@ include CHANGELOG.md
include LICENSE.txt
include README.md
recursive-include qtpy/tests *.py *.ui
include qtpy/py.typed
18 changes: 18 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,24 @@ conda install qtpy
```


### mypy

A CLI is offered to help with usage of QtPy including the subcommand
`qtpy mypy args` which generates command line arguments similar to the
following which help guide mypy through which library QtPy would have used
so that mypy can get the proper underlying type hints.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
A CLI is offered to help with usage of QtPy including the subcommand
`qtpy mypy args` which generates command line arguments similar to the
following which help guide mypy through which library QtPy would have used
so that mypy can get the proper underlying type hints.
A CLI is offered to help with usage of QtPy, including the subcommand
`qtpy mypy args`, which helps guide mypy through which library QtPy would have used
so that it can retrieve the proper underlying type hints.
This generates command line arguments similar to the following:

Fix some grammar and syntax issues and a run-on sentence, and a few diction refinements

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A CLI is offered to help with usage of QtPy which helps guide mypy through which library QtPy would have used so that it can retrieve the proper underlying type hints.

That's not really the message here though. Isolating out the middle bit doesn't help. I'll take a pass at improving this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if I've impaired the meaning, I was trying to break up a long run-on sentence and not have the "similar to the following" be disconnected from said "following". Your new revision looks much better than either, so we can go from there, thanks.


```
--always-false=PYQT4 --always-false=PYQT5 --always-false=PYSIDE --always-true=PYSIDE2
```

Use such as:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Use such as:
To use this, you can invoke mypy in the following way:

Fix grammar. Also, if something in the below is intended to be replaced, such as the path to the environment/QtPy, please say so, e.g. by adding something like the following: ", replacing <env/bin/qtpy> with the path to the QtPy installation:" Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand how that will help. If people know "the path to their qtpy installation" then they will know to change this. Also, do we need to cover -m qtpy and -m mypy and the various combinations? And if they just install system wide (--user or otherwise) then they wouldn't even be using a path.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess my intention was that its better to be explicit rather than implicit about what users are expected to replace, if the example is intended to be of practical value. However, given your point, perhaps its simpler to just specify the default invocations of the mypy and qtpy installed in the current environment (i.e. mypy and qtpy), which users can replace themselves with someone more specific if their situation requires it? Or is there some particular reason that mypy needs to be invoked with the full path?


```bash
env/bin/mypy --package mypackage $(env/bin/qtpy mypy args)
```


## Contributing

Everyone is welcome to contribute!
Expand Down
6 changes: 4 additions & 2 deletions qtpy/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -220,8 +220,10 @@ class PythonQtWarning(Warning):
warnings.warn('Selected binding "{}" could not be found, '
'using "{}"'.format(initial_api, API), RuntimeWarning)

API_NAME = {'pyqt5': 'PyQt5', 'pyqt': 'PyQt4', 'pyqt4': 'PyQt4',
'pyside': 'PySide', 'pyside2':'PySide2'}[API]
API_NAMES = {'pyqt5': 'PyQt5', 'pyqt': 'PyQt4', 'pyqt4': 'PyQt4',
'pyside': 'PySide', 'pyside2':'PySide2'}
APIS = sorted(name.upper() for name in set(API_NAMES.values()))
API_NAME = API_NAMES[API]

if PYQT4:
import sip
Expand Down
3 changes: 3 additions & 0 deletions qtpy/__main__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import qtpy.cli

qtpy.cli.cli()
Copy link
Member

@CAM-Gerlach CAM-Gerlach Mar 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
qtpy.cli.cli()
def main():
qtpy.cli.cli()
if __name__ == "__main__":
main()

A little extra boilerplate, but this allows pointing the setuptools entrypoint here (see below)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the major upside as compared with encouraging the hazardous behavior of making files both directly runnable and importable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid I don't follow the reasoning here. Could you explain why this behavior is more hazardous in this context of a trivial module following a well-known common pattern, relative to the practical risk of having separate console_scripts and runpy entrypoints that may behave differently or get out of sync? The if __name__ == "__main__" idiom is very common (the question explaining how and why to use it is the second most popular tagged python on all of SO), especially in the scientific Python ecosystem, and is recommended in the official Python docs and countless other sources.

Further, this particular pattern is recommended a substantial number of places, none of which describe any such issues with it.

Of course, there is the general principle of separation of concerns, and thus that entry points and package functionality should be separate (which this still fully ensures, as this module contains no non-trivial functionality beyond that necessary to serve as a single-source entrypoint for both console_scripts and runpy).

Beyond that, the only directly relevant post I found explicitly advocating against any use of if __name__ == __main__ was this personal blog, the one comment to which was a rebuttal by a Python core dev, and the (rather esoteric) potential harms mentioned don't apply to a module containing no non-trivial functionality beyond re-directing to the entry point. Some of these are at least theoretically possible if qtpy,cli were made runnable, but that's not what is being proposed here, and following your suggestions above, I rejected that. Therefore, I cannot see what the practical harm is, relative to the practical benefit (and avoidance of much more likely harm) of single-sourcing the entry piont.

42 changes: 42 additions & 0 deletions qtpy/cli.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import click


@click.group()
def cli():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def cli():
def cli(cli_args=None):

Wouldn't hurt to have this, in case we want to pass in arguments programmatically

"""Features in support of development with QtPy."""

pass


@cli.group()
def mypy():
"""Features in support of running mypy"""

pass


@mypy.command()
def args():
"""Generate command line arguments for using mypy with QtPy.

This will generate strings similar to the following which help guide mypy
through which library QtPy would have used so that mypy can get the proper
underlying type hints.

--always-false=PYQT4 --always-false=PYQT5 --always-false=PYSIDE --always-true=PYSIDE2

Use such as:

env/bin/mypy --package mypackage $(env/bin/qtpy mypy args)
"""

options = {False: '--always-false', True: '--always-true'}

import qtpy

apis_active = {name: qtpy.API.lower() == name.lower() for name in qtpy.APIS}
print(' '.join(
f'{options[is_active]}={name.upper()}'
for name, is_active
in apis_active.items()
))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
))
))
def main():
cli()
if __name__ == "__main__":
main()

A bit of boilerplate to make it a little easier to run as a script in case the user doesn't actually have QtPy installed, but rather just checked out locally

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really like encouraging not installing things. It makes it really hard to make things actually work. And why the indirection through main()? Would want a sys.exit()? Also, when would running this as a script work where running __main__.py or -m qtpy not work? Directly running files that are in packages is asking for a mess.

Copy link
Member

@CAM-Gerlach CAM-Gerlach Mar 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good points, agreed this isn't necessary or desirable. FWIW, if I'd started this package from scratch, I'd have put the top-level package inside a src directory, pulled the tests outside and run them against the installed package, to avoid any temptation to or accidentally run from the local copy, and verify package build and installation works correctly.

And why the indirection through main()?

Just convention, and probably a foolish one, at least in this case where it doesn't do anything special.

Would want a sys.exit()?

I assume its a moot point, but I'm curious as to the need for sys.exit()? Won't it exit anyway with 0 upon completion, or non-zero on error?

Empty file added qtpy/py.typed
Empty file.
45 changes: 45 additions & 0 deletions qtpy/tests/test_cli.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
from __future__ import absolute_import

import subprocess
import sys

import pytest

import qtpy


subcommands = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
subcommands = [
SUBCOMMANDS = [

As a top-level constant, shouldn't this be ALL_CAPS?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many people do that. I don't like it. No need to be YELLING. I presume it will get changed.

Copy link
Member

@CAM-Gerlach CAM-Gerlach Mar 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given Python has no runtime enforcement of constants staying constants, particularly for mutable objects like this list here which are effectively global state, it is particularly important to clearly distinguish them from regular variables. UPPER_CASE is the standard convention for global constants per PEP 8, Google style and pretty much every other Python style guide I'm aware of, is widely adopted in first and third-party packages, and is the convention used in this package.

['mypy'],
['mypy', 'args'],
]


@pytest.mark.parametrize(
argnames=['subcommand'],
argvalues=[[subcommand] for subcommand in subcommands],
ids=[' '.join(subcommand) for subcommand in subcommands],
Comment on lines +19 to +20
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
argvalues=[[subcommand] for subcommand in subcommands],
ids=[' '.join(subcommand) for subcommand in subcommands],
argvalues=[[subcommand] for subcommand in SUBCOMMANDS],
ids=[' '.join(subcommand) for subcommand in SUBCOMMANDS],

To match previous

)
def test_cli_help_does_not_fail(subcommand):
# .check_call() over .run(..., check=True) because of py2
subprocess.check_call(
[sys.executable, '-m', 'qtpy', *subcommand, '--help'],
)


def test_cli_mypy_args():
output = subprocess.check_output(
[sys.executable, '-m', 'qtpy', 'mypy', 'args'],
)

if qtpy.PYQT4:
expected = b'--always-true=PYQT4 --always-false=PYQT5 --always-false=PYSIDE --always-false=PYSIDE2\n'
elif qtpy.PYQT5:
expected = b'--always-false=PYQT4 --always-true=PYQT5 --always-false=PYSIDE --always-false=PYSIDE2\n'
elif qtpy.PYSIDE:
expected = b'--always-false=PYQT4 --always-false=PYQT5 --always-true=PYSIDE --always-false=PYSIDE2\n'
elif qtpy.PYSIDE2:
expected = b'--always-false=PYQT4 --always-false=PYQT5 --always-false=PYSIDE --always-true=PYSIDE2\n'
else:
assert False, 'No valid API to test'

assert output == expected
5 changes: 4 additions & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,5 +47,8 @@
'Programming Language :: Python :: 3',
'Programming Language :: Python :: 3.3',
'Programming Language :: Python :: 3.4',
'Programming Language :: Python :: 3.5']
'Programming Language :: Python :: 3.5'],
extras_require={'cli': ['click']},
entry_points={'console_scripts': 'qtpy = qtpy.cli:cli [cli]'},
Copy link
Member

@CAM-Gerlach CAM-Gerlach Mar 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
entry_points={'console_scripts': 'qtpy = qtpy.cli:cli [cli]'},
entry_points={'console_scripts': 'qtpy = qtpy.__main__:main'},

I've usually pointed my top-level entry point directly at __main__.main(), to ensure python -m qtpy and qtpy stay in sync and so only one place needs to be modified in case the cli file is moved, the function is renamed, or we point the entrypoint elsewhere, otherwise it could break one but not the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This demands a whole turn around that includes some icky stuff I'd rather just avoid. Not worth maybe having to update an entry point someday, maybe.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused with how changing a few characters here and adding a couple lines of standard boilerplate to __main__,py, both of which I made for you as suggestions, "demands a whole turn around" in your approach. Furthermore, as explained above, could you explain what practical harms this "icky stuff" has in a trivial module containing no actual logic that outweigh the non-trivial risk and cost of maintaining two parallel entrypoints?

include_package_data=True
)