-
-
Notifications
You must be signed in to change notification settings - Fork 153
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
PR: Support mypy #217
Conversation
I'm using this branch over in altendky/qtrio#83 if you are interested in seeing it running. It would be nice to add mypy runs in QtPy's CI as well. Is there any expected timeline for #208? Perhaps there is something I could help with to get it done? I'm just figuring I may as well add mypy to the new CI rather than the existing/old. Or, perhaps mypy checks against QtPy should just be added separately. |
Welp, there's piles of work in both PySide2 and PyQt5 hints but so far this seems to be ok. Any concerns here? Or anything I can do to help? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the very long delay; unfortunately, the main developer who was responsible for this left and resources were not re-allocated appropriately to maintain this critical package. While I don't actually use it myself (outside of as a Spyder developer, and I'm not that active anymore), given its clearly still well used by the community, I'll do my best to try to put out fires and get things moving along again, with your help. Thanks so much @altendky for your dedication! It really means a lot!
Overall, this seems like a fine change, if its needed to make things easier for mypy. However, instead of adding a new dependency on Click (and its peculiar syntax) just for a relatively trivial CLI, itself only for developer use with mypy, why not just use the standard-library argparse
? Other than that, I'm 👍 , once I get the CIs working again in PR #208 . Thanks!
Why Click? Because it is _so_ much nicer. But sure, I'll go ahead and switch this over to argparse. I've set this to draft and added notes in the OP listing what activities I should do prior to releasing this for review again. @CAM-Gerlach, thanks for finding some time here. If you want to tag me in on other tasks, let me know and I'll do whatever I can. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All sounds good! Just some minor nitpicks below. Hopefully we can get PR #208 merged soon, I just want to give @ccordoba12 a chance to look over it before going ahead and merging.
I'll let you know once that's merged, so you can rebase this accordingly.
README.md
Outdated
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. |
There was a problem hiding this comment.
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 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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
README.md
Outdated
--always-false=PYQT4 --always-false=PYQT5 --always-false=PYSIDE --always-true=PYSIDE2 | ||
``` | ||
|
||
Use such as: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
import qtpy | ||
|
||
|
||
subcommands = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
subcommands = [ | |
SUBCOMMANDS = [ |
As a top-level constant, shouldn't this be ALL_CAPS?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
argvalues=[[subcommand] for subcommand in subcommands], | ||
ids=[' '.join(subcommand) for subcommand in subcommands], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
qtpy/cli.py
Outdated
pass | ||
|
||
|
||
def cli(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def cli(): | |
def cli(cli_args=None): |
Wouldn't hurt to have this, in case we want to pass in arguments programmatically
qtpy/cli.py
Outdated
) | ||
mypy_args_parser.set_defaults(func=args) | ||
|
||
arguments = parser.parse_args() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
arguments = parser.parse_args() | |
arguments = parser.parse_args(args=cli_args) |
As above
f'{options[is_active]}={name.upper()}' | ||
for name, is_active | ||
in apis_active.items() | ||
)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
)) | |
)) | |
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
qtpy/__main__.py
Outdated
@@ -0,0 +1,3 @@ | |||
import qtpy.cli | |||
|
|||
qtpy.cli.cli() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
setup.py
Outdated
@@ -47,5 +47,7 @@ | |||
'Programming Language :: Python :: 3', | |||
'Programming Language :: Python :: 3.3', | |||
'Programming Language :: Python :: 3.4', | |||
'Programming Language :: Python :: 3.5'] | |||
'Programming Language :: Python :: 3.5'], | |||
entry_points={'console_scripts': 'qtpy = qtpy.cli:cli [cli]'}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
Yeah, I really like the idea of Click; I'm just used to argparse and its tended to be a decent fit for the stuff I build, which does have a lot of subcommands (albeit not sub-sub-commands) and I've developed patterns (albiet boilerplate-heavy) to work around most of its limitations. In this case, I didn't know you'd actually re-implement a bunch of Click's features within argparse for this relatively simple usecase, rather than just doing something simple; sorry for all the effort or I'd have just said to keep Click, ha! Our main priority right now is getting the backlog of PRs reviewed, tested and merged as soon as practical, so we can release 1.10 ASAP with the accumulated changes. Then, what I'm thinking might be a good tentative plan beyond that is to clean house and drop Py2 and Qt4, which we're no longer able to properly test due to package availability, add Qt6 support with PR #233 and release that shortly after as 2.0, but that's just my internal thinking, if @ccordoba12 and @stonebig agree. As for help, we'd welcome whatever you would like to contribute! We'd particularly welcome help addressing open issues that aren't already covered by PRs, or anything else you find, as well as giving feedback and testing on them. Once that's done, it will be much easier to work on things (esp things like adding Mypy annotations or type stubs, if you're thinking of that) without merge conflicts or things breaking. Maybe also add pre-commit to automate a good deal of the tedium of review and maintenance. Again, many thanks! |
Can we just go back to Click? It's not like it is a runtime dependency. It also is a pretty miniscule dependency compared with PyQt or PySide. It's even pure Python. Anyways, I'll go through all your notes. I generally also have reasons for doing things the way I do. But, the goal here is to build some momentum with QtPy. I was getting darn close to forking it... I'm glad there's renewed hope to keep the work here. For the hints, I think that mostly just passes through to PyQt and PySide. Other than the helpful hints for Mypy about what API is in use, as added here with the CLI. |
@CAM-Gerlach I'm so happy you move QtPy forward to Qt6, so I agree with anything. |
@stonebig Don't thank me haha, it was you and @jschueller that have done all the work on that front! I'm just here to help with maintenance, review, getting PRs moved forward and hopefully cutting releases. I was just about to ask on PR #225 , but I'd welcome your feedback on the tentative plan above; I'll comment on #225 so its more visible to everyone participating there. Thanks so much for your hard work! |
Actually I was thinking about that, given the considerable code complexity that using argparse entails given the (admittedly rather Click-centric) design choices made. However, upon further consideration, there are a few reasons this doesn't end up being a net win for simplicity, especially from a maintainer perspective:
If QtPy had a greater need for a general-purpose CLI, these (relatively modest) costs could be outweighed by its considerable benefits for such a use case, but as it stands, I just don't see how its worthwhile for a single, simple, developer-only command. However, much of the complexity (most notably having multiple layers of subcommands in subcommands) appears to be unnecessary or for the very simple use case presented here, of a single command returning a single string for consumption by another tool, intended for a small subset of developers. Simplifying it actually ends up being the same total length (0 net line change of code + tests) as the original version using Click, at minimal loss of meaningful functionality. See my repo for an implementation; feel free to pull that in to yours here or I can commit it directly to your branch, if you want. Also, I tweaked your
Yeah, my suggestions were just minor stuff: adding our standard file header, fixing a few grammar issues in the text, capitalizing a constant, and tweaking a few things about the entrypoint setup. I am curious for your input particularly on the last one—I've wondered if there's a reason/benefit to not pointing to Also, BTW keeping an eye on your ciborg project—would be really nice to have a CI abstraction layer; now that Conda-Smithy has Github Actions support I've been considering that, but as its pretty specialized a general tool would be much nicer. |
The nested subcommands were intended to lay out an organized interface that wouldn't require breaking changes if future tools are added. If hazarding such changes is preferred, certainly a single subcommand such as I'll provide feedback on the rest. It's sad that Conda, used to make it easier to get stuff, makes it harder to get stuff. |
Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
Good news, I went ahead and merged PR #208, so if you rebase you should get the tests running, and this should hopefully be close to ready.
Its not a bad approach, but with multiple layers of nested subcommands being a relatively uncommon design pattern for at least this ecosystem, increasing API, code and test complexity, and adding a new dev/test dependency with, all for a single specialized command seems a little overengineered to me (something I'm very much guilty of myself often). I'm also a little confused where the breaking changes come in; future tools could simply be added in separate subcommands, if the functionality cannot be added to the existing in a backward-compatible way.
Okay, thanks. I'll take a look momentarily.
Yeah, unfortunately the design decisions they've taken on that aspect have made it more difficult than it should be to implement optional dependencies in an easily maintainable manner. On the other hand, at least their build automation and tooling makes things that would be very painful and brittle with |
Hey, so unfortunately, by the time I was done reviewing and responding to the comments here, its almost 5am now and I need to eat and sleep, so I'll have to finish the actual review tomorrow, sorry. However, I did notice a number of things missing that were fixed in the linked branch on my fork, including:
I'm also unsure why the docstrings were removed; the previous method of parsing them directly from the called function was no more verbose and allowed them to be used both as docstrings and CLI help. However, other than that, this should be pretty close to ready. |
Just fix everything as you wish. |
Hey :) Sorry about making this kind of comment, but is there any progress on this? |
Hi @MinmoTech thank you for your interest! No for the moment but I think this is something we want to finish for sure in the future. Maybe after we release v2.1.0 we could take a look at this again and finish it. What do you think @CAM-Gerlach @ccordoba12 ? Also, just in case, is okay for you @altendky if we (probably @CAM-Gerlach or myself) finish this? Quite late to say but thank you so much for all the patience and time you spent working on this! |
Sure, I think this is useful. |
Sounds good to me, I have a branch on my fork implementing the requested changes (since @altendky gave the go-ahead above for us finishing it), but unfortunately life stuff intervened and I had to take a break from GitHub for a while. I'd been meaning to come back to finally finish it, I just hadn't quite gotten to it yet. If you agree, I can push that here with the merge conflicts fixed and any other updates, and we can proceed from there. Or, I can open a new PR, your call. |
I guess then merging/pushing your changes here should be okay @CAM-Gerlach , thanks! |
I ended up just opening a new PR, #337 , with the changes up until now squashed to a single commit to make rebasing easier, and then updated it with the various QtPy 2.x changes, fixed and improved the tests, simplified the CLI logic, refined the text/readme/help and added and tested a |
Ok, I'm going to close this PR then. Thanks for your help with this @altendky! |
Fixes #216.
Draft for: