-
Notifications
You must be signed in to change notification settings - Fork 55
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
Enforce strict Optional typing #723
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -217,7 +217,7 @@ def body(self, new_body: str) -> None: | |
self._body = new_body | ||
|
||
@property | ||
def id(self) -> int: | ||
def id(self) -> Optional[int]: | ||
return self._id | ||
|
||
@property | ||
|
@@ -226,12 +226,12 @@ def author(self) -> str: | |
return self._author | ||
|
||
@property | ||
def created(self) -> datetime.datetime: | ||
def created(self) -> Optional[datetime.datetime]: | ||
"""Datetime of creation of the comment.""" | ||
return self._created | ||
|
||
@property | ||
def edited(self) -> datetime.datetime: | ||
def edited(self) -> Optional[datetime.datetime]: | ||
"""Datetime of last edit of the comment.""" | ||
return self._edited | ||
|
||
|
@@ -256,7 +256,7 @@ def add_reaction(self, reaction: str) -> Reaction: | |
|
||
class IssueComment(Comment): | ||
@property | ||
def issue(self) -> "Issue": | ||
def issue(self) -> Optional["Issue"]: | ||
"""Issue of issue comment.""" | ||
return self._parent | ||
|
||
|
@@ -266,7 +266,7 @@ def __str__(self) -> str: | |
|
||
class PRComment(Comment): | ||
@property | ||
def pull_request(self) -> "PullRequest": | ||
def pull_request(self) -> Optional["PullRequest"]: | ||
"""Pull request of pull request comment.""" | ||
return self._parent | ||
|
||
|
@@ -642,12 +642,12 @@ def head_commit(self) -> str: | |
raise NotImplementedError() | ||
|
||
@property | ||
def target_branch_head_commit(self) -> str: | ||
def target_branch_head_commit(self) -> Optional[str]: | ||
"""Commit hash of the HEAD commit of the target branch.""" | ||
raise NotImplementedError() | ||
|
||
@property | ||
def merge_commit_sha(self) -> str: | ||
def merge_commit_sha(self) -> Optional[str]: | ||
""" | ||
Commit hash of the merge commit of the pull request. | ||
|
||
|
@@ -1014,7 +1014,7 @@ def created(self) -> datetime.datetime: | |
raise NotImplementedError() | ||
|
||
@property | ||
def edited(self) -> datetime.datetime: | ||
def edited(self) -> Optional[datetime.datetime]: | ||
"""Datetime of editing the commit status.""" | ||
raise NotImplementedError() | ||
|
||
|
@@ -1112,7 +1112,11 @@ def body(self) -> str: | |
|
||
@property | ||
def git_tag(self) -> GitTag: | ||
"""Object that represents tag tied to the release.""" | ||
"""Object that represents tag tied to the release. | ||
|
||
Raises: | ||
OgrException, if the tag is not found. | ||
""" | ||
raise NotImplementedError() | ||
|
||
@property | ||
|
@@ -1230,7 +1234,7 @@ class GitService(OgrAbstractClass): | |
instance_url (str): URL of the git forge instance. | ||
""" | ||
|
||
instance_url: Optional[str] = None | ||
instance_url: str | ||
|
||
def __init__(self, **_: Any) -> None: | ||
pass | ||
|
@@ -1326,7 +1330,9 @@ def list_projects( | |
|
||
|
||
class GitProject(OgrAbstractClass): | ||
def __init__(self, repo: str, service: GitService, namespace: str) -> None: | ||
def __init__( | ||
self, repo: str, service: GitService, namespace: Optional[str] | ||
) -> None: | ||
""" | ||
Args: | ||
repo: Name of the project. | ||
|
@@ -1335,7 +1341,7 @@ def __init__(self, repo: str, service: GitService, namespace: str) -> None: | |
|
||
- GitHub: username or org name. | ||
- GitLab: username or org name. | ||
- Pagure: namespace (e.g. `"rpms"`). | ||
- Pagure: namespace (e.g. `"rpms"`). May be `None`, i.e. no namespace present. | ||
Comment on lines
-1338
to
+1344
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about using an empty string for this? (I am looking at that from the I know, it's a functional change...;) |
||
|
||
In case of forks: `"fork/{username}/{namespace}"`. | ||
""" | ||
|
@@ -1916,7 +1922,7 @@ def get_username(self) -> str: | |
""" | ||
raise NotImplementedError() | ||
|
||
def get_email(self) -> str: | ||
def get_email(self) -> Optional[str]: | ||
""" | ||
Returns: | ||
Email of the user. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,7 +10,9 @@ | |
|
||
|
||
class TokenAuthentication(GithubAuthentication): | ||
def __init__(self, token: str, max_retries: Union[int, Retry] = 0, **_) -> None: | ||
def __init__( | ||
self, token: Optional[str], max_retries: Union[int, Retry] = 0, **_ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh no, memories of this magic are coming back /o\ |
||
) -> None: | ||
self._token = token | ||
self._pygithub_instance = github.Github(login_or_token=token, retry=max_retries) | ||
|
||
|
@@ -29,11 +31,11 @@ def __str__(self) -> str: | |
def pygithub_instance(self) -> github.Github: | ||
return self._pygithub_instance | ||
|
||
def get_token(self, namespace: str, repo: str) -> str: | ||
def get_token(self, namespace: str, repo: str) -> Optional[str]: | ||
return self._token | ||
|
||
@staticmethod | ||
def try_create( | ||
token: str = None, max_retries: Union[int, Retry] = 0, **_ | ||
token: Optional[str] = None, max_retries: Union[int, Retry] = 0, **_ | ||
) -> Optional["TokenAuthentication"]: | ||
return TokenAuthentication(token, max_retries=max_retries) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,7 +27,7 @@ | |
class GithubPullRequest(BasePullRequest): | ||
_raw_pr: _GithubPullRequest | ||
_target_project: "ogr_github.GithubProject" | ||
_source_project: "ogr_github.GithubProject" = None | ||
_source_project: Optional["ogr_github.GithubProject"] = None | ||
|
||
@property | ||
def title(self) -> str: | ||
|
@@ -148,17 +148,18 @@ def create( | |
github_repo = project.github_repo | ||
|
||
target_project = project | ||
if project.is_fork and fork_username is None: | ||
logger.warning(f"{project.full_repo_name} is fork, ignoring fork_repo.") | ||
source_branch = f"{project.namespace}:{source_branch}" | ||
github_repo = project.parent.github_repo | ||
target_project = project.parent | ||
elif fork_username: | ||
source_branch = f"{fork_username}:{source_branch}" | ||
if fork_username != project.namespace and project.parent is not None: | ||
github_repo = GithubPullRequest.__get_fork( | ||
fork_username, project.parent.github_repo | ||
) | ||
if project.parent: | ||
if project.is_fork and fork_username is None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is exactly one thing that I don't like on static typing :D it has no clue about invariants and integrity There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right, if it is a fork, then it definitely does have a parent but mypy doesn't know anything about it. I am not sure if we can somehow encode this relationship (I don't think so). One more check doesn't hurt and is safer... except that in some cases requre will shout :D |
||
logger.warning(f"{project.full_repo_name} is fork, ignoring fork_repo.") | ||
source_branch = f"{project.namespace}:{source_branch}" | ||
github_repo = project.parent.github_repo | ||
target_project = project.parent | ||
elif fork_username: | ||
source_branch = f"{fork_username}:{source_branch}" | ||
if fork_username != project.namespace and project.parent is not None: | ||
github_repo = GithubPullRequest.__get_fork( | ||
fork_username, project.parent.github_repo | ||
) | ||
|
||
created_pr = github_repo.create_pull( | ||
title=title, body=body, base=target_branch, head=source_branch | ||
|
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.
Does it make sense though?
I think I know what you meant in the description.
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.
Yes, this is a bit of a dilemma. It doesn't make much sense semantically, however just based on our interface, this could happen. If I wanted to enforce an
int
here, the order of arguments in the constructor would have to be changed (along with some other changes of course)