Skip to content

Conversation

@Avasam
Copy link
Collaborator

@AvasamAvasam commented Jan 5, 2023

These are way too complex, with 4 different possible sources (2 deprecated)
And we don't want to force the user to install PyQt or Pyside when they may not even use it.

Fixes 6 subclassing errors (the same error in Pillow, D3DShot, python-xlib, fpdf2, PyScreeze, and PyAutoGUI stubs)

Ref: #9491

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

Copy link
Collaborator

@hauntsaninjahauntsaninja left a comment

Choose a reason for hiding this comment

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

Maybe this should be a protocol?

@Avasam
Copy link
CollaboratorAuthor

Maybe this should be a protocol?

I thought about it, but ImageQt is used as a return type, so it'd have to completely match QtImage from all 4 implementations. I did a quick try and it got messy real quick with thousands of lines copied from PyQt6.

Protocol could probably be simplified by using Any when it starts referencing other types (so at least we have all methods and members). But would a protocol still work as a return type?

@AlexWaygood
Copy link
Member

Protocol could probably be simplified by using Any when it starts referencing other types (so at least we have all methods and members). But would a protocol still work as a return type?

Protocols can be used as a return type! I agree with @hauntsaninja that we can probably at least try to roughly describe the shape of the superclass here.

@AlexWaygoodAlexWaygood changed the title Explain why we sublass ImageQt with AnyExplain why we subclass ImageQt with AnyJan 5, 2023
@Avasam
Copy link
CollaboratorAuthor

Avasam commented Jan 5, 2023

I meant that I'm concerned about the variance:

fromtypingimportProtocolclassAbc: foo: strclassAbcProtocol(Abc, Protocol): ... # type: ignore[misc]reveal_type(AbcProtocol().foo). # strclassAbcd: foo: strabcd: Abcdabcd=AbcProtocol() # Incompatible types in assignment (abcproto: AbcProtocol=Abcd()

@hauntsaninja
Copy link
Collaborator

I'm not sure I follow, wouldn't users just use the QImage type hint?
(Or is it a case where users do actually know what type they're getting back and e.g. do nominal checking or the cases where the types' APIs are non-overlapping are actually important to users?)

@Avasam
Copy link
CollaboratorAuthor

Avasam commented Jan 7, 2023

Here's a minimal example in PyQt6. This is what we need to support (without actually having to install PyQt6, PyQt5, PySide6 or PySide2 when installing types-pillow). Would using a protocol for ImageQt's base work thanks to duck typing? And how complex would that protocol have to be? (QImages have lots of methods, members and bases).

fromPyQt6importQtGuifromPILimportImage, ImageQtdata: memoryview# Some graphics data from elsewhere in the codepil_image: Image.Image# Some pillow image from elsewhere in the codedefshow_qimage(image: QtGui.QImage): # Show the image someplace in the GUIpass# With PyQt directlyqimage=QtGui.QImage(data, 640, 480, 640*4, QtGui.QImage.Format.Format_RGBA8888) show_qimage(qimage) # using the pillow wrapper insteadqimage=ImageQt.ImageQt(pil_image) show_qimage(qimage) # Argument of type "ImageQt" cannot be assigned to parameter "image" of type "QImage" in function "show_qimage" "ImageQt" is incompatible with "QImage"

Would using a protocol for ImageQt's base work thanks to duck typing?

Edit: Don't think so

classA: deftest(self, foo: str) ->int: ... classProtocolA(Protocol): deftest(self, foo: str) ->int: ... protoA=cast(ProtocolA, A()) a: A=protoA# Expression of type "ProtocolA" cannot be assigned to declared type "A" "ProtocolA" is incompatible with "A"

@Avasam
Copy link
CollaboratorAuthor

@AlexWaygood , I don't see an "easy" solution for this (or any good solution at all). Since type-checkers can't have a conditional type based on if a module exists or not. (without having to check for a full ImportError, which can't be done statically, I feel like checking for a module's existence could be a beneficial proposition)

Can I move this to #9491 ?

@AlexWaygood
Copy link
Member

@AlexWaygood , I don't see an "easy" solution for this (or any good solution at all). Since type-checkers can't have a conditional type based on if a module exists or not. (without having to check for a full ImportError, which can't be done statically, I feel like checking for a module's existence could be a beneficial proposition)

Can I move this to #9491 ?

I'm fine with that. It's not like we're introducing a regression here, after all -- we're already sbuclassing Any in the stub. If anybody wants to invest some time into working out a more principled solution, they're always welcome to do so at a later date!

@Avasam
Copy link
CollaboratorAuthor

Closing in favor of #9491

@AvasamAvasam closed this Jan 12, 2023
@AvasamAvasam deleted the ImageQt-any-subclassing branch February 29, 2024 00:44
Sign up for freeto 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

@Avasam@AlexWaygood@hauntsaninja