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

Ensure types are consistent and turn on mypy #1162

Merged
merged 2 commits into from
Oct 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,14 @@ repos:
hooks:
- id: pyupgrade
args: [--py37-plus]
#- repo: https://github.com/pre-commit/mirrors-mypy
# rev: v0.910
# hooks:
# - id: mypy
# exclude: ^(docs/|tests/)
- repo: https://github.com/pre-commit/mirrors-mypy
rev: v1.5.1
hooks:
- id: mypy
additional_dependencies:
- types-python-dateutil
- types-requests
exclude: ^(docs/|tests/)
- repo: https://github.com/jorisroovers/gitlint
rev: v0.19.1
hooks:
Expand Down
7 changes: 7 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -86,3 +86,10 @@ exclude = '''
)/
)
'''

[tool.mypy]
files = ["."]
exclude = [
"^docs/",
"^tests/",
]
4 changes: 2 additions & 2 deletions src/github3/checks.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ def _update_attributes(self, pull):
def _repr(self):
return f"<CheckPullRequest [#{self.number}]>"

def to_pull(self):
def to_pull(self, conditional: bool = False):
Copy link
Owner

Choose a reason for hiding this comment

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

This should return -> t.Optional[pulls.PullRequest]

"""Retrieve a full PullRequest object for this CheckPullRequest.

:returns:
Expand Down Expand Up @@ -119,7 +119,7 @@ def _repr(self):
self.name, str(self.owner["login"])
)

def to_app(self):
def to_app(self, conditional: bool = False) -> models.GitHubCore:
"""Retrieve a full App object for this CheckApp.

:returns:
Expand Down
18 changes: 11 additions & 7 deletions src/github3/events.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ def _update_attributes(self, user):
self.login = user["login"]
self._api = self.url = user["url"]

def to_user(self):
def to_user(self, conditional: bool = False) -> models.GitHubCore:
"""Retrieve a full User object for this EventUser.

:returns:
Expand Down Expand Up @@ -93,7 +93,7 @@ def _update_attributes(self, org):
self.login = org["login"]
self._api = self.url = org["url"]

def to_org(self):
def to_org(self, conditional: bool = False) -> models.GitHubCore:
"""Retrieve a full Organization object for this EventOrganization.

:returns:
Expand Down Expand Up @@ -148,7 +148,7 @@ def _update_attributes(self, pull):
self.locked = pull["locked"]
self._api = self.url = pull["url"]

def to_pull(self):
def to_pull(self, conditional: bool = False) -> models.GitHubCore:
"""Retrieve a full PullRequest object for this EventPullRequest.

:returns:
Expand Down Expand Up @@ -258,7 +258,9 @@ def _update_attributes(self, comment):
self.updated_at = self._strptime(comment["updated_at"])
self.user = users.ShortUser(comment["user"], self)

def to_review_comment(self):
def to_review_comment(
self, conditional: bool = False
) -> models.GitHubCore:
Copy link
Owner

Choose a reason for hiding this comment

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

This is too generic. We can be more specific here.

"""Retrieve a full ReviewComment object for this EventReviewComment.

:returns:
Expand All @@ -269,7 +271,7 @@ def to_review_comment(self):
from . import pulls

comment = self._json(self._get(self._api), 200)
return pulls.ReviewComment(comment, self)
return pulls.ReviewComment(comment, self.session)

refresh = to_review_comment

Expand All @@ -285,7 +287,7 @@ def _update_attributes(self, issue):
self.locked = issue["locked"]
self._api = self.url = issue["url"]

def to_issue(self):
def to_issue(self, conditional: bool = False) -> models.GitHubCore:
"""Retrieve a full Issue object for this EventIssue."""
from . import issues

Expand Down Expand Up @@ -352,7 +354,9 @@ def _update_attributes(self, comment):
self.updated_at = self._strptime(comment["updated_at"])
self.user = users.ShortUser(comment["user"], self)

def to_issue_comment(self):
def to_issue_comment(
self, conditional: bool = False
) -> models.GitHubCore:
"""Retrieve the full IssueComment object for this comment.

:returns:
Expand Down
7 changes: 5 additions & 2 deletions src/github3/gists/gist.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
"""This module contains the Gist, ShortGist, and GistFork objects."""
import typing as t
from json import dumps

from . import comment
Expand Down Expand Up @@ -31,7 +32,9 @@ class _Gist(models.GitHubCore):
"""

class_name = "_Gist"
_file_class = gistfile.ShortGistFile
_file_class: t.Type[
t.Union[gistfile.ShortGistFile, gistfile.GistFile]
] = gistfile.ShortGistFile

def _update_attributes(self, gist):
self.comments_count = gist["comments"]
Expand Down Expand Up @@ -265,7 +268,7 @@ def _update_attributes(self, fork):
def _repr(self):
return f"<GistFork [{self.id}]>"

def to_gist(self):
def to_gist(self, conditional: bool = False) -> models.GitHubCore:
"""Retrieve the full Gist representation of this fork.

:returns:
Expand Down
2 changes: 1 addition & 1 deletion src/github3/gists/history.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ class GistHistory(models.GitHubCore):
def _update_attributes(self, history) -> None:
self.url = self._api = history["url"]
self.version = history["version"]
self.user = users.ShortUser(history["user"], self)
self.user = users.ShortUser(history["user"], self.session)
self.change_status = history["change_status"]
self.additions = self.change_status.get("additions")
self.deletions = self.change_status.get("deletions")
Expand Down
72 changes: 36 additions & 36 deletions src/github3/git.py
Original file line number Diff line number Diff line change
Expand Up @@ -297,42 +297,6 @@ def _repr(self):
return f"<Tag [{self.tag}]>"


class CommitTree(models.GitHubCore):
"""This object represents the abbreviated tree data in a commit.

The API returns different representations of different objects. When
representing a :class:`~github3.git.ShortCommit` or
:class:`~github3.git.Commit`, the API returns an abbreviated
representation of a git tree.

This object has the following attributes:

.. attribute:: sha

The SHA1 of this tree in the git repository.
"""

def _update_attributes(self, tree):
self._api = tree["url"]
self.sha = tree["sha"]

def _repr(self):
return f"<CommitTree [{self.sha}]>"

def to_tree(self):
"""Retrieve a full Tree object for this CommitTree.

:returns:
The full git data about this tree
:rtype:
:class:`~github3.git.Tree`
"""
json = self._json(self._get(self._api), 200)
return self._instance_or_null(Tree, json)

refresh = to_tree


class Tree(models.GitHubCore):
"""This represents a tree object from a git repository.

Expand Down Expand Up @@ -382,6 +346,42 @@ def recurse(self):
return self._instance_or_null(Tree, json)


class CommitTree(models.GitHubCore):
Copy link
Owner

Choose a reason for hiding this comment

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

Why are you moving this definition? That seems unnecessary

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 is because of a bug in mypy, let me find the reference.

"""This object represents the abbreviated tree data in a commit.

The API returns different representations of different objects. When
representing a :class:`~github3.git.ShortCommit` or
:class:`~github3.git.Commit`, the API returns an abbreviated
representation of a git tree.

This object has the following attributes:

.. attribute:: sha

The SHA1 of this tree in the git repository.
"""

def _update_attributes(self, tree):
self._api = tree["url"]
self.sha = tree["sha"]

def _repr(self):
return f"<CommitTree [{self.sha}]>"

def to_tree(self, conditional: bool = False) -> models.GitHubCore:
"""Retrieve a full Tree object for this CommitTree.

:returns:
The full git data about this tree
:rtype:
:class:`~github3.git.Tree`
"""
json = self._json(self._get(self._api), 200)
return self._instance_or_null(Tree, json)

refresh = to_tree


class Hash(models.GitHubCore):
"""This is used to represent the elements of a tree.

Expand Down
4 changes: 2 additions & 2 deletions src/github3/github.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import re
import typing as t

import uritemplate
import uritemplate # type: ignore

from . import apps
from . import auths
Expand Down Expand Up @@ -544,7 +544,7 @@ def authorize(
@requires_auth
def blocked_users(
self, number: int = -1, etag: t.Optional[str] = None
) -> t.Generator[users.ShortUser, None, None]:
) -> t.Iterator[users.ShortUser]:
"""Iterate over the users blocked by this organization.

.. versionadded:: 2.1.0
Expand Down
2 changes: 1 addition & 1 deletion src/github3/issues/issue.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
"""Module containing the Issue logic."""
from json import dumps

from uritemplate import URITemplate
from uritemplate import URITemplate # type: ignore

from . import comment
from . import event
Expand Down
25 changes: 12 additions & 13 deletions src/github3/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,6 @@
LOG = logging.getLogger(__package__)


T = t.TypeVar("T")


class GitHubCore:
"""The base object for all objects that require a session.

Expand All @@ -28,7 +25,7 @@ class GitHubCore:
"""

_ratelimit_resource = "core"
_refresh_to: t.Optional["GitHubCore"] = None
_refresh_to: t.Optional[t.Type["GitHubCore"]] = None

def __init__(self, json, session: session.GitHubSession):
"""Initialize our basic object.
Expand Down Expand Up @@ -240,26 +237,28 @@ def _api(self):
value += f"?{self._uri.query}"
return value

@staticmethod
def _uri_parse(uri):
return requests.compat.urlparse(uri)

@_api.setter
def _api(self, uri):
if uri:
self._uri = self._uri_parse(uri)
self.url = uri

@staticmethod
def _uri_parse(uri):
Copy link
Owner

Choose a reason for hiding this comment

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

Again, there's no point or value apparent in moving this

Copy link
Contributor Author

@sarahmonod sarahmonod Oct 10, 2023

Choose a reason for hiding this comment

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

This is because mypy cannot deal with @property getters and setters not being right next to each other: python/mypy#1465

Copy link
Owner

Choose a reason for hiding this comment

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

Well that's truly infuriating

return requests.compat.urlparse(uri)

def _iter(
self,
count: int,
url: str,
cls: t.Type[T],
params: t.Optional[t.Mapping[str, t.Optional[str]]] = None,
cls: t.Type["GitHubCore"],
Copy link
Owner

Choose a reason for hiding this comment

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

Technically this is true, but it's not accurate. _iter returns a GitHubIterator that returns a given type, which we can tie to cls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh good point, let me see if I can fix that.

params: t.Optional[
t.MutableMapping[str, t.Union[str, int, None]]
] = None,
etag: t.Optional[str] = None,
headers: t.Optional[t.Mapping[str, str]] = None,
list_key: t.Optional[str] = None,
) -> "structs.GitHubIterator[T]":
) -> "structs.GitHubIterator":
"""Generic iterator for this project.

:param int count: How many items to return.
Expand All @@ -276,7 +275,7 @@ def _iter(
from .structs import GitHubIterator

return GitHubIterator(
count, url, cls, self, params, etag, headers, list_key
count, url, cls, self.session, params, etag, headers, list_key
)

@property
Expand Down Expand Up @@ -329,7 +328,7 @@ def refresh(self, conditional: bool = False) -> "GitHubCore":
self._json_data = json
self._update_attributes(json)
else:
return self._refresh_to(json, self)
return self._refresh_to(json, self.session)
return self

def new_session(self):
Expand Down
8 changes: 4 additions & 4 deletions src/github3/orgs.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
import typing as t
from json import dumps

from uritemplate import URITemplate
from uritemplate import URITemplate # type: ignore

from . import models
from . import users
Expand Down Expand Up @@ -211,7 +211,7 @@ def permissions_for(
headers = {"Accept": "application/vnd.github.v3.repository+json"}
url = self._build_url("repos", repository, base_url=self._api)
json = self._json(self._get(url, headers=headers), 200)
return ShortRepositoryWithPermissions(json, self)
return ShortRepositoryWithPermissions(json, self.session)

@requires_auth
def repositories(self, number=-1, etag=None):
Expand Down Expand Up @@ -491,7 +491,7 @@ def add_repository(self, repository, team_id): # FIXME(jlk): add perms
@requires_auth
def blocked_users(
self, number: int = -1, etag: t.Optional[str] = None
) -> t.Generator[users.ShortUser, None, None]:
) -> t.Iterator[users.ShortUser]:
Copy link
Owner

Choose a reason for hiding this comment

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

I believe t.Iterator is likely going to cause issues as GitHubIterator has publicly documented additional methods which this type does/will not reveal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean you expect people to be using methods from t.Generator or GitHubIterator?

Copy link
Owner

Choose a reason for hiding this comment

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

I mean the latter. The old annotation was certainly also wrkng

"""Iterate over the users blocked by this organization.

.. versionadded:: 2.1.0
Expand Down Expand Up @@ -749,7 +749,7 @@ def create_team(
getattr(r, "full_name", r) for r in (repo_names or [])
],
"maintainers": [
getattr(m, "login", m) for m in (maintainers or [])
str(getattr(m, "login", m)) for m in (maintainers or [])
Copy link
Owner

Choose a reason for hiding this comment

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

Why is this necessary but getattr(r, "full_name", r) isn't?

],
"permission": permission,
"privacy": privacy,
Expand Down
2 changes: 1 addition & 1 deletion src/github3/pulls.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
"""This module contains all the classes relating to pull requests."""
from json import dumps

from uritemplate import URITemplate
from uritemplate import URITemplate # type: ignore

from . import models
from . import users
Expand Down
Empty file added src/github3/py.typed
Empty file.
Loading
Loading