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

Add all qflags bases classes #153

Merged
merged 230 commits into from
Dec 15, 2021
Merged

Conversation

bluebird75
Copy link
Collaborator

I started from my work on windowFlags and look for ways to make it generic. I failed at defining a generic approach with mypy, protocols and generics inheriting from int, it is really above my understanding of mypy.

But, it's easy enough to generate a file for each QFlag based class which can be used to assert both the actual implementation types and values, and which ensures that the class is being correctly checked. This is what I am doing here for two classes : WindowFlags and Alignment.

With a few more hours, I should be able to cover all qflags based class. This would remove a big hole in PyQt stubs.

bluebird75 and others added 24 commits March 26, 2021 15:53
Before the fix, the test would report:
windowflags.py:10: error: Incompatible types in assignment (expression has type "int", variable has type "WindowType")
windowflags.py:18: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags")
windowflags.py:19: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags")
windowflags.py:20: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags")
windowflags.py:21: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags")
windowflags.py:22: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags")
windowflags.py:27: error: Unsupported operand types for + ("WindowFlags" and "WindowType")
windowflags.py:27: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags")
windowflags.py:28: error: Unsupported operand types for - ("WindowFlags" and "WindowType")
windowflags.py:28: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags")
windowflags.py:29: error: Unsupported operand types for | ("WindowFlags" and "WindowType")
windowflags.py:29: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags")
windowflags.py:30: error: Unsupported operand types for & ("WindowFlags" and "WindowType")
windowflags.py:30: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags")
windowflags.py:31: error: Unsupported operand types for ^ ("WindowFlags" and "WindowType")
windowflags.py:31: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags")
windowflags.py:34: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags")
windowflags.py:34: error: Unsupported operand types for + ("WindowType" and "WindowFlags")
windowflags.py:35: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags")
windowflags.py:35: error: Unsupported operand types for - ("WindowType" and "WindowFlags")
windowflags.py:36: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags")
windowflags.py:36: error: Unsupported operand types for | ("WindowType" and "WindowFlags")
windowflags.py:37: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags")
windowflags.py:37: error: Unsupported operand types for & ("WindowType" and "WindowFlags")
windowflags.py:38: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags")
windowflags.py:38: error: Unsupported operand types for ^ ("WindowType" and "WindowFlags")
windowflags.py:41: error: Unsupported left operand type for + ("WindowFlags")
windowflags.py:42: error: Unsupported left operand type for - ("WindowFlags")
windowflags.py:43: error: Unsupported left operand type for | ("WindowFlags")
windowflags.py:44: error: Unsupported left operand type for & ("WindowFlags")
windowflags.py:45: error: Unsupported left operand type for ^ ("WindowFlags")
Breaking Liskov subsitution is OK because that's what PyQt5 does
Copy link
Collaborator

@altendky altendky left a comment

Choose a reason for hiding this comment

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

I'll suggest this quickly now, in case you are around and we can get the CI fixed. I'll go back to reviewing the test code now.

PyQt5-stubs/QtCore.pyi Outdated Show resolved Hide resolved
Co-authored-by: Kyle Altendorf <[email protected]>
@bluebird75
Copy link
Collaborator Author

The file where I would be interested for your review is qflags_aligmentflags.py . It uses a mostly generic approach on QFlag and QFlags based classes to check all operations on both mypy and python side.

Only two lines are specific to AlignementFlag in this file. I have work in progress to generate one similar file per QFlag based class (for the testing side) and applying automatically a fix to the pyi files to add the missing methods.

@altendky altendky dismissed their stale review August 13, 2021 15:05

requested change was made

@bluebird75
Copy link
Collaborator Author

This is all green finally. Code is ready to be merged.

@bluebird75 bluebird75 mentioned this pull request Nov 20, 2021
@bluebird75
Copy link
Collaborator Author

Hi Kyle. Can you start reviewing that one ? It is the result of a long work so I would be thrilled to have it integrated.

Copy link
Collaborator

@altendky altendky left a comment

Choose a reason for hiding this comment

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

Several remaining nitpicks about the use of pathlib (which I do very much like). A general note I didn't point out everywhere is that I use path not filename for pathlib.Paths. Feel free to merge after going through the suggestions.

For another PR, it would be good for the generation to be run in every CI run and for there to be a CI failure if the generation results in any changes, additions, or removals.

tests/test_stubs.py Outdated Show resolved Hide resolved
tests/test_stubs.py Outdated Show resolved Hide resolved
tests/test_stubs.py Outdated Show resolved Hide resolved
tests/test_stubs.py Outdated Show resolved Hide resolved
tests/test_stubs.py Outdated Show resolved Hide resolved
tests/test_stubs.py Outdated Show resolved Hide resolved
tests/test_stubs.py Outdated Show resolved Hide resolved
tests/test_stubs.py Outdated Show resolved Hide resolved
tests/test_stubs.py Outdated Show resolved Hide resolved
@bluebird75
Copy link
Collaborator Author

Thanks for the feedback. This read_text() was unknown to me and I appreciated the other ideas. The code was not written with top standards but to achieve a useful purpose. It probably could be improved.

It is not really feasible to run the generation on CI. First, it is very time consuming because libcst needs to parse the huge PyQt5 modules for each QFlag generation. Second, the source of the QFlag information is a manual grep over Qt sources, which requires some user modifications before being a proper source for complete generation.

But it's a good idea to run it on new Qt or PyQt releases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants