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

get true PR pushed_at datetime #271

Merged
merged 5 commits into from
May 26, 2017
Merged

get true PR pushed_at datetime #271

merged 5 commits into from
May 26, 2017

Conversation

amoffat
Copy link
Contributor

@amoffat amoffat commented May 26, 2017 •
edited
Loading

This should fix a number of issues, but it needs sanity checking. Underlying issue mentioned #199 and #147 and #239 (comment)

@PlasmaPower
Copy link
Contributor

PlasmaPower commented May 26, 2017 •
edited
Loading

Slightly sketchy, but it should work. see review

PlasmaPower
PlasmaPower suggested changes May 26, 2017
View reviewed changes
github_api/prs.py Outdated
#
# using all of these facts, we are able to determine the true, reliable,
# last update time of the branch backing a PR
github_test_merge_commit = pr_data["merge_commit_sha"]
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't exist until GitHub is done checking for conflicts. This function should return None in that case, and the callers should be updated accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Beat me to it 😜

reddraggone9
reddraggone9 reviewed May 26, 2017
View reviewed changes
github_api/prs.py Outdated
# using all of these facts, we are able to determine the true, reliable,
# last update time of the branch backing a PR
github_test_merge_commit = pr_data["merge_commit_sha"]
commit = commits.get_commit(api, urn, github_test_merge_commit)
Copy link
Contributor

Choose a reason for hiding this comment

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

The linked docs don't say so, but is it seems possible that there might not be a "merge_commit_sha" before GitHub has tested for mergeability or even while it's doing so. Since you don't account for that, have you tested this?

@amoffat amoffat changed the title get true PR pushed_at datetime WIP get true PR pushed_at datetime May 26, 2017
@PlasmaPower
Copy link
Contributor

Slight warning: https://developer.github.com/changes/2013-04-25-deprecating-merge-commit-sha/

@reddraggone9
Copy link
Contributor

@PlasmaPower That post says at the top they decided not to deprecate it for the REST API. Or is that supposed to really slight warning since they had it deprecated in the past?

@reddraggone9 reddraggone9 mentioned this pull request May 26, 2017
Closed
@PlasmaPower
Copy link
Contributor

PlasmaPower commented May 26, 2017 •
edited
Loading

@reddraggone9 Pretty much that that. GitHub considers the API not well designed, so it might be deleted with the v4 api or something.

@amoffat
Copy link
Contributor Author

amoffat commented May 26, 2017

Alright, handling merge_commit_sha not being there/being empty, and added a note about a potential deprecation in the future

@amoffat amoffat changed the title WIP get true PR pushed_at datetime get true PR pushed_at datetime May 26, 2017
@reddraggone9
Copy link
Contributor

Neat. Comments have some grammar issues (oh hey, you caught one while I was typing!), but it looks good. 👍

@reddraggone9
Copy link
Contributor

reddraggone9 commented May 26, 2017 •
edited
Loading

get_pr_last_updated didn't previously hit the API since it used data that had already been fetched, but it now does. Given the number of places it's called, this is probably going to do a number to our rate limit. I forsee much cooldown sleeping in our future if this is merged as is.

@amoffat
Copy link
Contributor Author

amoffat commented May 26, 2017

You make an api request, and you make an api request...

oprah's github api client

@chaosbot chaosbot merged commit ef69d70 into Chaosthebot:master May 26, 2017
@chaosbot
Copy link
Collaborator

🙆‍♀️ PR passed with a vote of 11 for and 0 against, with a weighted total of 11.0 and a threshold of 6.1.

See merge-commit ef69d70 for more details.

@amoffat amoffat deleted the separate-branches branch May 26, 2017 08:26
@amoffat
Copy link
Contributor Author

amoffat commented May 26, 2017

Seems like there are some issues with relying on merge_commit_sha #293

@amoffat amoffat mentioned this pull request May 26, 2017
Closed
chaosbot added a commit that referenced this pull request May 26, 2017
@chaosbot
Revert "merging PR #271: get true PR pushed_at datetime"
e8cbc33 
This reverts commit ef69d70, reversing
changes made to c2498c4.

this is a manual revert due to the nature of the bug it causes (making it nearly
impossible for PRs to get merged in)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reddraggone9 reddraggone9 reddraggone9 left review comments

@PlasmaPower PlasmaPower PlasmaPower requested changes

@bengjerstad bengjerstad bengjerstad approved these changes

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@amoffat @PlasmaPower @reddraggone9 @chaosbot @bengjerstad