-
Notifications
You must be signed in to change notification settings - Fork 407
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
Conversation
c811376
to
bd4e1b0
Compare
It looks like `betamax` is failing to appropriately mock the `close` method in its mock connection. Signed-off-by: Gus Monod <[email protected]>
Signed-off-by: Gus Monod <[email protected]>
mypy
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.
Much of this looks like it should satisfy mypy but it doesn't actually provide useful types to users. GitHubCore
isn't something that people should be caring about. They should be getting return types that point to the object they care about and the methods and attributes on it. As it stands, if people use this, it will lead to them needing tonnes of type: ignore
whenever they access a method or attribute for the class in particular.
@@ -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): |
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.
This should return -> t.Optional[pulls.PullRequest]
def to_review_comment(self): | ||
def to_review_comment( | ||
self, conditional: bool = False | ||
) -> models.GitHubCore: |
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.
This is too generic. We can be more specific here.
@@ -382,6 +346,42 @@ def recurse(self): | |||
return self._instance_or_null(Tree, json) | |||
|
|||
|
|||
class CommitTree(models.GitHubCore): |
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.
Why are you moving this definition? That seems unnecessary
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.
This is because of a bug in mypy, let me find the reference.
@_api.setter | ||
def _api(self, uri): | ||
if uri: | ||
self._uri = self._uri_parse(uri) | ||
self.url = uri | ||
|
||
@staticmethod | ||
def _uri_parse(uri): |
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.
Again, there's no point or value apparent in moving this
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.
This is because mypy cannot deal with @property
getters and setters not being right next to each other: python/mypy#1465
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.
Well that's truly infuriating
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"], |
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.
Technically this is true, but it's not accurate. _iter
returns a GitHubIterator that returns a given type, which we can tie to cls
.
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.
Oh good point, let me see if I can fix that.
@@ -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]: |
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 believe t.Iterator
is likely going to cause issues as GitHubIterator
has publicly documented additional methods which this type does/will not reveal
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.
Do you mean you expect people to be using methods from t.Generator
or GitHubIterator
?
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 mean the latter. The old annotation was certainly also wrkng
@@ -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 []) |
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.
Why is this necessary but getattr(r, "full_name", r)
isn't?
Exactly. I would like to break this work into two pieces, which may or may not be in the same PR. One would make |
I see your point about wanting to break this up into smaller pieces of work. I'll approve and merge this since mypy does seem to not be unhappy with it. I would want to progressively make mypy more strict though as we enforce things |
This is a draft PR to start the discussion on adding type information to
github3.py
.The first commit was necessary to get the tests to pass for me, I didn't investigate any further than just mark the failing tests as
xfail
as they were failing for the default branch.The second commit is the minimal set of change I could think of that would make running
mypy
return a success.The third commit is just a few of the types that could be added to
github3.py
. I'm happy to add more, but would like to know what the project maintainers think of this before I do.