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 Type Hints to image/__init__.py #818

Closed
wants to merge 11 commits into from
Closed

Conversation

gran4
Copy link
Contributor

@gran4 gran4 commented Apr 30, 2023

I wanted to fix #787 so I type hinted it.

pyglet/image/__init__.py Fixed Show fixed Hide fixed
@gran4
Copy link
Contributor Author

gran4 commented May 1, 2023

empty

pyglet/image/__init__.py Fixed Show fixed Hide fixed
Copy link
Member

@benmoran56 benmoran56 left a comment

Choose a reason for hiding this comment

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

Most of this looks good, but there are a large amount of inaccuracies. I have made comments for those that I have found, but there could be more.

pyglet/image/__init__.py Outdated Show resolved Hide resolved
pyglet/image/__init__.py Outdated Show resolved Hide resolved
pyglet/image/__init__.py Outdated Show resolved Hide resolved
pyglet/image/__init__.py Show resolved Hide resolved
pyglet/image/__init__.py Show resolved Hide resolved
pyglet/image/__init__.py Outdated Show resolved Hide resolved
pyglet/image/__init__.py Outdated Show resolved Hide resolved
pyglet/image/__init__.py Outdated Show resolved Hide resolved
pyglet/image/__init__.py Outdated Show resolved Hide resolved
pyglet/image/__init__.py Outdated Show resolved Hide resolved
@gran4 gran4 requested a review from benmoran56 May 14, 2023 15:28
pyglet/image/__init__.py Outdated Show resolved Hide resolved
pyglet/image/__init__.py Outdated Show resolved Hide resolved
pyglet/image/__init__.py Outdated Show resolved Hide resolved
pyglet/image/__init__.py Outdated Show resolved Hide resolved
pyglet/image/__init__.py Outdated Show resolved Hide resolved
pyglet/image/__init__.py Outdated Show resolved Hide resolved
@benmoran56
Copy link
Member

Pardon the delay, but it's quite challenging to review this PR. There are many mistakes, and it takes a lot of time to review manually. I added a few more comments for issues that I found. There are also several issues that appear all throughout, but I did not comment them.

  1. Many of the classmethods are wrong. cls should be the actual object of the class that will be returned, but it's often shown as AbstractImage. This might pass a type checker, but it's returning the subclass is more accurate.

  2. There are many unnecessary docstring breaks. All docstrings should start on the same line as the triple quotes for compactness. For example:

def function():
    """This is preferred

def function():
    """
    Not this
  1. Many functions are unnecessarily wrapped. pyglet adheres to PEP8, with the exception of a relaxed line length limit. It's acceptable (and preferred) to allow function definitions to reach 100 (or up to 110) columns, rather than splitting to multiple lines.

@gran4
Copy link
Contributor Author

gran4 commented May 25, 2023 via email

pyglet/image/__init__.py Fixed Show fixed Hide fixed
pyglet/image/__init__.py Fixed Show fixed Hide fixed
Comment on lines +385 to +388
def save(self, filename: str= "",
file: Optional[IO]=None,
encoder: Optional[str]=None
) -> Optional[ImageEncodeException]:

Check notice

Code scanning / CodeQL

Explicit returns mixed with implicit (fall through) returns Note

Mixing implicit and explicit returns may indicate an error as implicit returns always return None.
@benmoran56 benmoran56 closed this Sep 16, 2023
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.

cannot blit_into image
2 participants