Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from all commits
0a72a91
52c6039
80dda11
bbf4247
59c8ab7
203d4f4
d38fcab
91e5834
7754e75
4a3aeb4
135b74c
b279790
b05c7c7
3d171a5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 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 asys.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.Just convention, and probably a foolish one, at least in this case where it doesn't do anything special.
I assume its a moot point, but I'm curious as to the need for
sys.exit()
? Won't it exit anyway with0
upon completion, or non-zero on error?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.
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.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.
To match previous