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

fix: fix math with timeline and commit counts of a PR #19

Merged

Conversation

hicksjacobp
Copy link
Contributor

From our Enterprise Server at 3.4.2, we found cases where a pull request has
more than 250 commits. It seems like the timeline count doesn't incorporate the
full commit count, so the subtraction of commit count from the timeline count
would result in a negative total record count.

This commit changes the math of the issue event count so that only 250 commits
are subtracted from the timeline count if the commit count is greater than 250.
This logic and behavior of the timeline GraphQL property needs to be confirmed
by a GitHub engineer.

NOTE: Similar changes may need to be made with respect to the comment count of
a pull request as well. I just haven't found a use case to test it myself.

I'm leaving this in a draft state at the moment, because I'm not sure if this
accurate or not. @admiralAwkbar - this is what I emailed you about and a
potential fix.

From our Enterprise Server at 3.4.2, we found cases where a pull request has
more than 250 commits. It seems like the timeline count doesn't incorporate the
full commit count, so the subtraction of commit count from the timeline count
would result in a negative total record count.

This commit changes the math of the issue event count so that only 250 commits
are subtracted from the timeline count if the commit count is greater than 250.
This logic and behavior of the timeline GraphQL property needs to be confirmed
by a GitHub engineer.

NOTE: Similar changes may need to be made with respect to the comment count of
a pull request as well. I just haven't found a use case to test it myself.
@admiralAwkbar
Copy link
Contributor

@hicksjacobp Hey great catch on this one...
YEah with the changes across the apis, and various edge cases, its hard to really keep it all...
But I think this is a great safeguard to help get more accurate numbers..

@hicksjacobp hicksjacobp marked this pull request as ready for review May 10, 2022 12:41
@hicksjacobp hicksjacobp requested a review from a team as a code owner May 10, 2022 12:41
@hicksjacobp
Copy link
Contributor Author

@admiralAwkbar I moved this to ready for review. I don't have an example where there could be 250+ PR review comments (separate from specific comments on a file/line), so I couldn't really test for that and left the TODO in. If you have any other suggestions, please let me know or feel free to contribute edits to my branch.

@admiralAwkbar admiralAwkbar merged commit 7f549c1 into mona-actions:main May 10, 2022
@hicksjacobp hicksjacobp deleted the hicksjacobp/fix-event-count branch May 10, 2022 13:55
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.

2 participants