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: wip fix total commit counts #211

Merged
merged 8 commits into from
Jul 31, 2020
Merged

fix: wip fix total commit counts #211

merged 8 commits into from
Jul 31, 2020

Conversation

anuraghazra
Copy link
Owner

@anuraghazra anuraghazra commented Jul 27, 2020

fixes #14
fixes #92
closes #15

Currently github readme stats can only fetch total commits of 1year max, because of limitations of Github API mentioned in here #14 (comment) but we can fix it by brute forcing and looping over all the years to accumulate total commits of a user as we did in #15

But there are downsides of this approach in terms on performance because we are looping over all the years and calling the GraphQL Api each time

The fix

So as mentioned by @kingthorin in this comment #15 (comment) instead of showing "Total commits" we would show "Total Commits (2020)" in order to eliminate any confusion.

And as mentioned in this #92 (comment) by @adamyi Github has a experimental REST endpoint to fetch all the commits which we can utilize nicely.

We've added a new flag "include_all_commits" which would fetch the total commits of a user if enabled otherwise we will show only the last year commits (which is current behaviour)

Screenshots

@rampatra issue #92

  • &include_all_commits=true
    rampatra_true

  • &include_all_commits=false
    rampatra_false

@anshumanv issue #14

  • &include_all_commits=true

anshu_true

  • &include_all_commits=false
    anshu_false

NOTE: As mentioned in github's documentation https://developer.github.com/v3/search/#search-commits this API is experimental and the API may change without advance notice during the preview period.

TODO:

  • Write tests
  • Discussion

@anuraghazra anuraghazra added bug Something isn't working. enhancement New feature or request. help wanted Extra attention is needed. in-progress Currently working on. stats-card Feature, Enhancement, Fixes related to stats the stats card. labels Jul 27, 2020
@codecov
Copy link

codecov bot commented Jul 27, 2020

Codecov Report

Merging #211 into master will decrease coverage by 0.57%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #211      +/-   ##
==========================================
- Coverage   94.86%   94.28%   -0.58%     
==========================================
  Files          16       16              
  Lines         467      490      +23     
  Branches      123      130       +7     
==========================================
+ Hits          443      462      +19     
- Misses         20       24       +4     
  Partials        4        4              
Impacted Files Coverage Δ
src/fetchStats.js 91.30% <81.81%> (-8.70%) ⬇️
api/index.js 100.00% <100.00%> (ø)
src/renderStatsCard.js 100.00% <100.00%> (ø)

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 a4486d0...ff292f2. Read the comment docs.

@rampatra
Copy link

I think when &include_all_commits=true the year shouldn't be there in the title Total Commits (2020). It should be just Total Commits.

@anshumanv
Copy link

Doesn't show up in mine for some reason

@anuraghazra
Copy link
Owner Author

I think when &include_all_commits=true the year shouldn't be there in the title Total Commits (2020). It should be just Total Commits.

Oh weird, maybe i uploaded the wrong screenshot. my bad

@anuraghazra
Copy link
Owner Author

anuraghazra commented Jul 27, 2020

Live demo

include_all_commits=false

include_all_commits=true

include_all_commits=false

include_all_commits=true

@masterking32
Copy link

Hey @anuraghazra , thanks, Can we have both of them? like this
Total commits: XXX
2020 Commits: XXX

@anuraghazra
Copy link
Owner Author

Hey @anuraghazra , thanks, Can we have both of them? like this
Total commits: XXX
2020 Commits: XXX

Good idea, but i'm not sure i would do that, it seems kinda redundant.

@bokub
Copy link
Contributor

bokub commented Aug 14, 2020

Just to be sure about this code:

stats.totalCommits =
contributionCount.totalCommitContributions + experimental_totalCommits;
if (count_private) {
stats.totalCommits += contributionCount.restrictedContributionsCount;
}

I though experimental_totalCommits already contained all the commits, but you're adding the totalCommitContributions and the restrictedContributionsCount from the GraphQL API.

Aren't you counting the same commits 2 times ?

@anuraghazra
Copy link
Owner Author

Just to be sure about this code:

stats.totalCommits =
contributionCount.totalCommitContributions + experimental_totalCommits;
if (count_private) {
stats.totalCommits += contributionCount.restrictedContributionsCount;
}

I though experimental_totalCommits already contained all the commits, but you're adding the totalCommitContributions and the restrictedContributionsCount from the GraphQL API.

Aren't you counting the same commits 2 times ?

#298 feel free to send a PR

@bokub
Copy link
Contributor

bokub commented Aug 14, 2020

Sure! I will try to send a PR this weekend if I find some time

Edit: see #391

mochoy added a commit to mochoy/mochoy that referenced this pull request Sep 8, 2020
total commits current has a bug where it's not properly counting commits, but it gives me a better rating lmao. Hoping for fix soon: 
anuraghazra/github-readme-stats#211
anuraghazra/github-readme-stats#298
@anuraghazra anuraghazra deleted the fix-total-commits branch October 31, 2020 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working. enhancement New feature or request. help wanted Extra attention is needed. in-progress Currently working on. stats-card Feature, Enhancement, Fixes related to stats the stats card.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Total commits shown on the card is way less than actual Bug: Wrong commit count
5 participants