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
Closed

PR: Support mypy #217

wants to merge 14 commits into from

Conversation

altendky
Copy link
Contributor

@altendky altendky commented Jul 8, 2020

Fixes #216.

Draft for:

  • Convert to argparse for the CLI
  • Self review and retest since this has been sitting for awhile
  • Wait for PR: Move CI to Github Actions #208 to be merged so we have CI for these changes

@altendky altendky marked this pull request as draft July 8, 2020 20:53
@altendky
Copy link
Contributor Author

altendky commented Jul 9, 2020

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.

@altendky altendky mentioned this pull request Jul 9, 2020
15 tasks
@altendky altendky marked this pull request as ready for review July 12, 2020 02:55
@altendky
Copy link
Contributor Author

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?

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a 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!

@altendky altendky marked this pull request as draft March 19, 2021 13:16
@altendky
Copy link
Contributor Author

Why Click? Because it is _so_ much nicer. :] And, as you say, it doesn't need to be (and isn't) a runtime dependency. Some programs are mostly about doing one thing with a few options, but a QtPy CLI seems more likely to be made up of miscellaneous helpers for different activities. This lends itself very much to subcommands which aren't the most pleasant thing in argparse.

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.

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a 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.

qtpy/cli.py Show resolved Hide resolved
README.md Outdated
Comment on lines 76 to 79
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.

README.md Outdated
--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?

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.

Comment on lines +19 to +20
argvalues=[[subcommand] for subcommand in subcommands],
ids=[' '.join(subcommand) for subcommand in 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
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():
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

qtpy/cli.py Outdated
)
mypy_args_parser.set_defaults(func=args)

arguments = parser.parse_args()
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
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()
))
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?

qtpy/__main__.py Outdated
@@ -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.

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]'},
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?

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Mar 20, 2021

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!

@altendky
Copy link
Contributor Author

Can we just go back to Click? :] It's massively simpler and doesn't involve forgoing features nor recreating them. Though, it only took a few minutes for this. I do also have my own argparse patterns I've made, which these came from. Mostly only used in my own env-bootstrapper that of course generally ends up installing Click so everything else can use it.

https://github.com/altendky/boots/blob/9d399eed1fb7448dd3be7e707a787202f1755918/boots.py#L1018-L1171

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.

@stonebig
Copy link
Contributor

stonebig commented Mar 20, 2021

@CAM-Gerlach I'm so happy you move QtPy forward to Qt6, so I agree with anything.

@CAM-Gerlach
Copy link
Member

@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!

@CAM-Gerlach
Copy link
Member

Can we just go back to Click? :] It's massively simpler and doesn't involve forgoing features nor recreating them.

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:

  • Conda (through which many if not most of our users get QtPy) does not support optional dependencies; there are workarounds but they add significant effort and complexity for maintainers, users or both; see Optional groups of dependencies conda/conda#7502
  • It will require some feedstock changes to get the conda builds working, at a minimum adding click as a test-only dependency (I think that's possible to do), on top of whatever approach we take to resolve the above. I don't have direct access there, so it could delay a conda-forge release.
  • It isn't used in any other Spyder-related project (nor the broader scientific Python ecosystem) and none of us are really familiar with it, making things more difficult in the event of a bug, breaking change or new adding a new command.

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 args() function to actually return its args so it can be called programmatically, and renamed it more descriptively as a verb following standard PEP 8 convention. I've also fixed a bug in the tests on Windows in a separate commit.

I generally also have reasons for doing things the way I do.

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 __main__ for the main top-level entrypoint, that would outweigh the downsides mentioned.

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.

@altendky
Copy link
Contributor Author

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 qtpy mypy-args could be used instead.

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. :[

@CAM-Gerlach
Copy link
Member

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.

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 qtpy mypy-args could be used instead.

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.

I'll provide feedback on the rest.

Okay, thanks. I'll take a look momentarily.

It's sad that Conda, used to make it easier to get stuff, makes it harder to get stuff. :[

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 pip/setuptools/wheel relatively straightforward.

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Mar 23, 2021

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:

  • fixing the tests on Windows
  • making mypy_args actually return the args so they could be retrieved programmatically (without hacks) and then simply wrapping that in a print lambda in cli(),
  • Resolving grammar errors in the docstring
  • handling the default argv properly
  • making the function names verbs.

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.

@altendky
Copy link
Contributor Author

Just fix everything as you wish.

@dalthviz dalthviz changed the title Support mypy PR: Support mypy Aug 7, 2021
@dalthviz dalthviz modified the milestones: v1.10.0, v2.0.0 Aug 10, 2021
@dalthviz dalthviz removed this from the v2.0.0 milestone Nov 1, 2021
@MinmoTech
Copy link

Hey :)

Sorry about making this kind of comment, but is there any progress on this?
It seems like a great addition!

@dalthviz
Copy link
Member

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!

@ccordoba12
Copy link
Member

Sure, I think this is useful.

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Apr 15, 2022

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.

@dalthviz
Copy link
Member

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!

@CAM-Gerlach
Copy link
Member

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 --version arg.

@ccordoba12
Copy link
Member

Ok, I'm going to close this PR then. Thanks for your help with this @altendky!

@ccordoba12 ccordoba12 closed this Apr 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support type hints and mypy
6 participants