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.

Sign up for GitHub

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

Jump to bottom

Add proper type annotations (and some refactorings) #18

Merged
merged 8 commits into from
Jun 14, 2023
Merged

Add proper type annotations (and some refactorings) #18

merged 8 commits into from
Jun 14, 2023

Conversation

Advik-B
Copy link
Contributor

@Advik-B Advik-B commented Jun 13, 2023 •
edited by zkoppert
Loading

This pull request adds type annotations and updates function docstrings.

@Advik-B Advik-B requested a review from zkoppert as a code owner June 13, 2023 16:38
@zkoppert zkoppert added the documentation Improvements or additions to documentation label Jun 13, 2023
zkoppert
zkoppert reviewed Jun 13, 2023
View reviewed changes
issue_metrics.py Outdated
Comment on lines 30 to 31
# IssueWithMetrics is a type alias for a GitHub issue with metrics attached.
IssueWithMetrics = github3.issues.Issue
Copy link
Member

@zkoppert zkoppert Jun 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Python is seeing this as a duplicate declaration of IssueWithMetric on line 357. We might just need to move up the class definition to replace these lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait, ill push a commit to fix this

Copy link
Contributor Author

@Advik-B Advik-B Jun 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
# IssueWithMetrics is a type alias for a GitHub issue with metrics attached.
IssueWithMetrics = github3.issues.Issue
from github3.issues.issue import Issue as IssueWithMetrics

Does this look good? I am directly renaming the github3.Issue type in the import statement itslf
I think this will fix the issue that python is complaining about...

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's accurate to say that IssueWithMetrics is of type github3.issues.issue. I've moved the definition of issueWithMetrics earlier in the file so it can be referenced earlier. That seems to have resolved the issue.

@Advik-B
Copy link
Contributor Author

Advik-B commented Jun 14, 2023

Update: Sourcery AI provided some refactorings, ive included them, for the code explainations see Advik-B#1

@Advik-B Advik-B changed the title Add proper type annotations Add proper type annotations (and some refactorings) Jun 14, 2023
@zkoppert
simplify some logic
f250cae 
Signed-off-by: Zack Koppert <[email protected]>
zkoppert
zkoppert approved these changes Jun 14, 2023
View reviewed changes
Copy link
Member

@zkoppert zkoppert left a comment

Choose a reason for hiding this comment

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

🚀

@zkoppert zkoppert merged commit 21b637b into github:main Jun 14, 2023
@zkoppert zkoppert mentioned this pull request Jul 21, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zkoppert zkoppert zkoppert approved these changes

Assignees
No one assigned
Labels
documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Advik-B @zkoppert