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

Update documentation of count_private option #2504

Closed
wants to merge 5 commits into from
Closed

Update documentation of count_private option #2504

wants to merge 5 commits into from

Conversation

harry-graham
Copy link

For #2500.

@vercel
Copy link

vercel bot commented Feb 7, 2023

@harry-graham is attempting to deploy a commit to the github readme stats Team on Vercel.

A member of the Team first needs to authorize it.

readme.md Show resolved Hide resolved
readme.md Show resolved Hide resolved
Copy link
Collaborator

@Zo-Bro-23 Zo-Bro-23 left a comment

Choose a reason for hiding this comment

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

I'm okay with these changes. I think it would be better if we could update the actual logic behind the code too, along with the documentation update, though. It seems a fairly simple change - just add the private contributions to the totalContributions value instead of the totalCommits value. @rickstaa what do you think?

Adds restrictedContributionsCount to stats.contributedTo instead of stats.totalCommits
Update documentation after c90c194
readme.md Show resolved Hide resolved
Copy link
Collaborator

@Zo-Bro-23 Zo-Bro-23 left a comment

Choose a reason for hiding this comment

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

Looks good to me! I've updated some code logic too, so everything hopefully makes sense now. @rickstaa let me know what you think.

@Zo-Bro-23 Zo-Bro-23 added documentation Improvements or additions to documentation. design Issues, fixes related to design or alignments. stats-card Feature, Enhancement, Fixes related to stats the stats card. labels Feb 9, 2023
@harry-graham
Copy link
Author

Hey @Zo-Bro-23, just wanted to double-check with you: I think contributedTo represents the total number of repositories contributed to, so I'm not sure it makes sense to add the total number of private contributions to that.

Example

Suppose a person has contributed to 3 public repositories and 3 private repositories.
Suppose that person has done 100 contributions in each of those 6 repositories.

After this PR, we would have:

  • When count_private: false => contributedTo == 3
  • When count_private: true => contributedTo == 303

What are your thoughts?

@Zo-Bro-23
Copy link
Collaborator

Hey @Zo-Bro-23, just wanted to double-check with you: I think contributedTo represents the total number of repositories contributed to, so I'm not sure it makes sense to add the total number of private contributions to that.

Example

Suppose a person has contributed to 3 public repositories and 3 private repositories. Suppose that person has done 100 contributions in each of those 6 repositories.

After this PR, we would have:

  • When count_private: false => contributedTo == 3
  • When count_private: true => contributedTo == 303

What are your thoughts?

You're completely right 🤦 I'm sorry to have caused all this confusion. Another readme stats card I use has a field called totalContributions, which is why I initially mistook contributedTo for totalContributions. In this case, I think it would be best if we removed the count_private option. @rickstaa @anuraghazra what do you think? The restrictedContributions field gives private contributions, which is not something that we currently have. Else, we can add totalContributions as a field and add the restrictedContributions value to that, but for the time being, I think we should just remove the option altogether.

PS: I've reverted my commits.

@codecov
Copy link

codecov bot commented Feb 9, 2023

Codecov Report

Base: 97.25% // Head: 97.25% // No change to project coverage 👍

Coverage data is based on head (4729f02) compared to base (1120006).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2504   +/-   ##
=======================================
  Coverage   97.25%   97.25%           
=======================================
  Files          24       24           
  Lines        4152     4152           
  Branches      380      380           
=======================================
  Hits         4038     4038           
  Misses        112      112           
  Partials        2        2           
Impacted Files Coverage Δ
src/fetchers/stats-fetcher.js 93.14% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@harry-graham
Copy link
Author

I'm sorry to have caused all this confusion.

No worries, thanks for being so active on this PR, and for helping to manage the back-and-forth, I really appreciate it! 🙌🏼

@Zo-Bro-23
Copy link
Collaborator

I'm sorry to have caused all this confusion.

No worries, thanks for being so active on this PR, and for helping to manage the back-and-forth, I really appreciate it! 🙌🏼

We'll wait for @anuraghazra and @rickstaa before making any more moves!

@rickstaa
Copy link
Collaborator

rickstaa commented Feb 14, 2023

@harry-graham, thanks for bringing this issue to our attention.

I agree that the count_private parameter is misleading. There is currently no way to get a user's private commits without deploying a private Vercel instance (see https://docs.github.com/en/graphql/reference/objects). I, therefore, like @Zo-Bro-23 think it is best to remove this parameter and the accompanying code entirely. The documentation can then be improved so that it is clear that people need to deploy their instance if they want to include private commits. @anuraghazra, what do you think?

@harry-graham
Copy link
Author

Closing this off for now.

@harry-graham harry-graham deleted the update-count-private-documentation branch February 18, 2023 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design Issues, fixes related to design or alignments. documentation Improvements or additions to documentation. stats-card Feature, Enhancement, Fixes related to stats the stats card.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Github Stats card: Private commits count actually counts private contributions
3 participants