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

Support github timeline events (GH-422). #1302

Merged
merged 1 commit into from Dec 28, 2019

Conversation

ncb000gt
Copy link
Contributor

@ncb000gt ncb000gt commented Nov 26, 2019

Adds support for retrieval of Github timeline events (GH-422).

Usage example:

issue = repo.get_issue(28)
events = issue.get_timeline()
for event in events:
    print(f'Event: {event}')

The class doesn't match the output exactly as shown on the api site because the output from the call/events doesn't. This can be seen in the text file used for testing. This can be verified using the following.

curl -i https://[REPLACE WITH GH TOKEN]@api.github.com/repositories/3544490/issues/28/timeline -H"Accept:application/vnd.github.mockingbird-preview"

Signed-off-by: Nick Campbell nicholas.j.campbell@gmail.com

@TravisBuddy
Copy link

Hey @ncb000gt,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: a01bd840-1057-11ea-a65a-5f455ab5a299

@codecov-io
Copy link

codecov-io commented Nov 26, 2019

Codecov Report

Merging #1302 into master will decrease coverage by 0.03%.
The diff coverage is 94.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1302      +/-   ##
==========================================
- Coverage   98.17%   98.14%   -0.04%     
==========================================
  Files         186      188       +2     
  Lines       14092    14222     +130     
==========================================
+ Hits        13835    13958     +123     
- Misses        257      264       +7
Impacted Files Coverage Δ
tests/Issue.py 100% <100%> (ø) ⬆️
github/Consts.py 100% <100%> (ø) ⬆️
github/Issue.py 98.61% <100%> (+0.01%) ⬆️
github/TimelineEventSource.py 90% <90%> (ø)
github/TimelineEvent.py 93.82% <93.82%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update baddb71...d27ea9a. Read the comment docs.

@s-t-e-v-e-n-k
Copy link
Collaborator

This is looking great! Would you mind adding a test case for TimelineEvent where you fetch one from the API and assert it's parsed correctly? tests/RequiredStatusChecks.py is a small example.

@ncb000gt
Copy link
Contributor Author

ncb000gt commented Dec 2, 2019

@s-t-e-v-e-n-k I'll take a look and post back. Thanks.

@ncb000gt
Copy link
Contributor Author

ncb000gt commented Dec 3, 2019

@s-t-e-v-e-n-k I took a look and the difference between individual timeline events is that they can't be lazily loaded. You either get the full timeline or you don't. I removed the "load if not found" calls since they can't be individually loaded. I added some additional checks to the test that I did add.

What are your thoughts?

@TravisBuddy
Copy link

Hey @ncb000gt,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 5c00a040-1585-11ea-81b2-273600e26419

@s-t-e-v-e-n-k
Copy link
Collaborator

I'm happy with the replay data including the entire timeline even if you only assert the attributes of the first one. You can always selectively edit the replay data to have more brevity.

tests/Issue.py Outdated Show resolved Hide resolved
@ncb000gt ncb000gt force-pushed the master branch 2 times, most recently from 1c420ab to 4e26c3b Compare December 12, 2019 15:48
@TravisBuddy
Copy link

Hey @ncb000gt,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 0ec12420-1cf7-11ea-a2fb-51cb67500490

@ncb000gt
Copy link
Contributor Author

@s-t-e-v-e-n-k let me know if you want me to make any additional changes. Thanks!

Copy link
Collaborator

@s-t-e-v-e-n-k s-t-e-v-e-n-k left a comment

Choose a reason for hiding this comment

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

Two more niggling issues now that I've had a close look.

github/TimelineEventSource.py Outdated Show resolved Hide resolved
tests/Issue.py Outdated Show resolved Hide resolved
@TravisBuddy
Copy link

Hey @ncb000gt,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: f34db3e0-2208-11ea-b76a-c509dfe0ae07

@TravisBuddy
Copy link

Hey @ncb000gt,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 4bbb87e0-220a-11ea-b76a-c509dfe0ae07

github/TimelineEvent.py Outdated Show resolved Hide resolved
import github.TimelineEventSource


class TimelineEvent(github.GithubObject.NonCompletableGithubObject):
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is completable! It has a URL and everything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They don't all have a url. Some do. if you look through the replay data the cross-referenced event does not have a url.

Adds support for retrieval of Github timeline events.

Signed-off-by: Nick Campbell <nicholas.j.campbell@gmail.com>
@TravisBuddy
Copy link

Hey @ncb000gt,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 7a6f7eb0-2265-11ea-b76a-c509dfe0ae07

@s-t-e-v-e-n-k s-t-e-v-e-n-k merged commit 732fd26 into PyGithub:master Dec 28, 2019
@sfdye sfdye mentioned this pull request Apr 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants