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 Webhook Deliveries #2508

Merged
merged 6 commits into from Jun 20, 2023
Merged

Add Webhook Deliveries #2508

merged 6 commits into from Jun 20, 2023

Conversation

jmgreg31
Copy link
Contributor

Webhook Deliveries

Add support for Organization and Repository Webhook Deliveries

Summary of Changes

  • Added two methods to MainClass, Organization, and Repository to support get_hook_delivery and get_hook_deliveries
  • Created a HookDelivery model where github.HookDelivery.HookDelivery is a subclass of github.HookDelivery.HookDeliverySummary for DRY implementation
  • Added test cases to Organization and Repository for validation

Resolves #2507

@codecov-commenter
Copy link

codecov-commenter commented Apr 23, 2023 •
edited

Codecov Report

Patch coverage: 97.54% and project coverage change: -0.01 ⚠️

Comparison is base (804c310) 98.33% compared to head (d00e9ed) 98.32%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #2508      +/-   ##
==========================================
- Coverage   98.33%   98.32%   -0.01%     
==========================================
  Files         130      131       +1     
  Lines       12978    13140     +162     
==========================================
+ Hits        12762    12920     +158     
- Misses        216      220       +4
Impacted Files Coverage Δ
github/HookDelivery.py 97.01% <97.01%> (ø)
github/GithubObject.py 96.42% <100.00%> (ø)
github/MainClass.py 99.29% <100.00%> (+0.02%) ⬆️
github/Organization.py 98.83% <100.00%> (+0.01%) ⬆️
github/Repository.py 97.15% <100.00%> (+0.01%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@trim21
Copy link
Contributor

trim21 commented May 13, 2023

Can we write inline type annotation instead of seprated pyi for new file?

#2481

@jmgreg31 jmgreg31 force-pushed the feature/jmgreg-webhook-deliveries branch 5 times, most recently from cab4757 to 934681e Compare May 15, 2023 14:04
@jmgreg31
Copy link
Contributor Author

jmgreg31 commented May 15, 2023 •
edited

Can we write inline type annotation instead of seprated pyi for new file?

#2481

@trim21 This ended up being a bit more difficult trying to bridge the gap from existing .pyi structure. I believe I was able to get this implemented correctly, but had to make some updates to other .pyi file dependancies to pass linting across all python versions. Apologies if folks were getting emailed about all the CI builds while testing, I eventually realized I was getting the same mypy linting errors locally and was able to continue validation without triggering the CI

EnricoMi
EnricoMi requested changes May 23, 2023
View reviewed changes
Copy link
Collaborator

@EnricoMi EnricoMi left a comment

Choose a reason for hiding this comment

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

Excellent pull request, can we extend the tests please? I think the replay data are already sufficient, just more assertions.

tests/Organization.py Show resolved Hide resolved
github/MainClass.py Show resolved Hide resolved
@jmgreg31
Copy link
Contributor Author

@EnricoMi Added assertions for all attributes in Organization and Repository as well as adding the same replay/test workflow to Github_.py to test MainClass directly

@jmgreg31 jmgreg31 force-pushed the feature/jmgreg-webhook-deliveries branch from 08c0f35 to acb8a62 Compare May 26, 2023 03:55
@jmgreg31 jmgreg31 requested a review from EnricoMi May 31, 2023 02:46
@jmgreg31 jmgreg31 force-pushed the feature/jmgreg-webhook-deliveries branch 2 times, most recently from 5765c0b to a2428df Compare June 10, 2023 10:43
@EnricoMi EnricoMi force-pushed the feature/jmgreg-webhook-deliveries branch from a2428df to 16ffa39 Compare June 20, 2023 05:48
@EnricoMi EnricoMi changed the title feature: Webhook Deliveries Add Webhook Deliveries Jun 20, 2023
@EnricoMi
Copy link
Collaborator

@jmgreg31 sorry for the delay. We have reworked some related code, which broke your pull request. I have resolved the issues and changed delivered_at into a datetime attribute.

EnricoMi
EnricoMi approved these changes Jun 20, 2023
View reviewed changes
Copy link
Collaborator

@EnricoMi EnricoMi left a comment

Choose a reason for hiding this comment

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

LGTM!

trim21
trim21 reviewed Jun 20, 2023
View reviewed changes
github/PaginatedList.py Outdated
@@ -88,7 +90,7 @@ def __finished(self, index):
return self.__stop is not None and index >= self.__stop


class PaginatedList(PaginatedListBase):
class PaginatedList(PaginatedListBase, Generic[T]):
Copy link
Contributor

Choose a reason for hiding this comment

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

this generic is not actually used in this pr？

Copy link
Collaborator

Choose a reason for hiding this comment

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

resolved

@EnricoMi EnricoMi merged commit 517ad33 into PyGithub:master Jun 20, 2023
10 checks passed
@jmgreg31
Copy link
Contributor Author

jmgreg31 commented Jun 20, 2023 •
edited

Thanks for the reviews and merge! @EnricoMi @trim21

@jmgreg31 jmgreg31 deleted the feature/jmgreg-webhook-deliveries branch June 20, 2023 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trim21 trim21 trim21 left review comments

@EnricoMi EnricoMi EnricoMi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ENHANCEMENT: Support for Webhook Deliveries
4 participants
@jmgreg31 @codecov-commenter @trim21 @EnricoMi