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

Feature team stats #3182

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pursonchen
Copy link

Hi, guys. This day I needed to evaluate a development team's GitHub status, and then I found your awesome tools, The card is very good, I made a little change to aggregate many accounts's statuses and then output a status card. the API is https://host/api?team=account1,account2

@vercel
Copy link

vercel bot commented Aug 31, 2023

Someone is attempting to deploy a commit to the github readme stats Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions github-actions bot added ranks Feature, Bug fix, improvement related to ranking system. dependencies Pull requests that update a dependency file labels Aug 31, 2023
@rickstaa
Copy link
Collaborator

@pursonchen Although I love the feature you have implemented, we can, in the current state, not merge it into the master branch. This is because it performs N GraphQL requests, causing our already overloaded public instance rate-limited faster (i.e. https://docs.github.com/en/graphql/overview/resource-limitations) 😅. See #1471 for more information. I would, however, like to add your card under an environmental variable when we release our GitHub action later this year (i.e. #2179). Could you create a feature request so we can track your feature and see how much interest there is from the community?

Comment on lines +73 to +90
for (let username of username_list) {
const stats = await fetchStats(
username,
parseBoolean(include_all_commits),
parseArray(exclude_repo),
);
team_stats.totalPRs += stats.totalPRs;
team_stats.totalPRsMerged += stats.totalPRsMerged;
team_stats.mergedPRsPercentage += stats.mergedPRsPercentage;
team_stats.totalReviews += stats.totalReviews;
team_stats.totalCommits += stats.totalCommits;
team_stats.totalIssues += stats.totalIssues;
team_stats.totalStars += stats.totalStars;
team_stats.totalDiscussionsStarted += stats.totalDiscussionsStarted;
team_stats.totalDiscussionsAnswered += stats.totalDiscussionsAnswered;
team_stats.contributedTo += stats.contributedTo;
team_stats.rank.percentile += stats.rank.percentile;
}
Copy link
Collaborator

@qwerty541 qwerty541 Sep 14, 2023

Choose a reason for hiding this comment

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

@pursonchen I think that the following lines of code should be totally reworked if we are going to merge this feature someday. I expect to have only one GraphQL request which will fetch required data about team members. Calling fetchStats() function inside loop is an error since it decrease performace and cause hitting upstream GitHub API rate limits. Algorithm complexity should be improved from $O(n)$ to $O(1)$ as previously noticed @rickstaa.

I also do not see much sense in showing sum stats of n people and their average rank. Is showing rank useful in case of team card? I do not think so since our median and weight values inside rank calculation algorithm (see https://github.com/anuraghazra/github-readme-stats/blob/master/src/calculateRank.js) are intended specially for users. I think it is a place for discussion.

Remember that you can implement anything and deploy it on your own Vercel instance.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@qwerty541, excellent point! It seems that with GraphQL v4, data for multiple users can now be fetched without using more GraphQL points:

{
  user1: user(login: "rickstaa") {
    ...UserFragment
  }
  user2: user(login: "qwerty541") {
    ...UserFragment
  }
  user3: user(login: "pursonchen") {
    ...UserFragment
  }
  rateLimit {
    limit
    cost
    remaining
    resetAt
  }
}

fragment UserFragment on User {
  login
  name
  contributionsCollection {
    totalCommitContributions
    totalPullRequestReviewContributions
  }
  repositoriesContributedTo(
    first: 1
    contributionTypes: [COMMIT, ISSUE, PULL_REQUEST, REPOSITORY]
  ) {
    totalCount
  }
  pullRequests(first: 1) {
    totalCount
  }
  mergedPullRequests: pullRequests(states: MERGED) {
    totalCount
  }
  openIssues: issues(states: OPEN) {
    totalCount
  }
  closedIssues: issues(states: CLOSED) {
    totalCount
  }
  followers {
    totalCount
  }
  repositoryDiscussions {
    totalCount
  }
  repositoryDiscussionComments(onlyAnswers: true) {
    totalCount
  }
}

I was surprised to learn about that feature and that it only uses 1 GraphQL point. Thanks for bringing it to my attention. As a result, making the addition of this card possible without having to wait for the GitHub action to be released.

However, creating a separate team data fetcher is essential so that the codebase doesn't get cluttered. Furthermore, we have to discuss which stats would be interesting to show for teams. I think the following stats are not helpful when looking at teams:

  • Contributed to (last year).
  • Total discussions started.
  • Total discussion answered.
  • PR reviews.

@github-actions github-actions bot added the ⭐ top pull request Top pull request. label Sep 25, 2023
@github-actions github-actions bot removed the ⭐ top pull request Top pull request. label Oct 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file ranks Feature, Bug fix, improvement related to ranking system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants