-
Notifications
You must be signed in to change notification settings - Fork 8
Adding the points allocated to users in a PR comment #131
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request introduces an automated contributor points tracking system that monitors PR activity and displays point allocations in PR comments. The system replaces a previous leaderboard-based approach with real-time point tracking directly on PRs, calculating points for reviews, approvals, merged PRs, and various bonus categories like bug fixes, security improvements, and documentation contributions.
Key Changes:
- Implements a comprehensive Python-based points tracking script that monitors multiple GitHub events (reviews, comments, PR lifecycle events)
- Replaces file-based leaderboard updates with dynamic PR comment updates showing live point tracking
- Expands point categories to include performance improvements, security fixes, and first-time contributor bonuses
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 18 comments.
| File | Description |
|---|---|
| scripts/track_points.py | New 520-line Python script implementing the core points calculation logic, GitHub API integration, and PR comment management |
| scripts/config_points.yml | Updated configuration file expanding point categories from 4 to 10+ types, adding security, performance, and documentation bonuses |
| .github/workflows/points.yml | Refactored workflow to trigger on more event types (pull_request, issues, etc.) and execute the new tracking script instead of creating leaderboard PRs |
| @@ -1,29 +1,37 @@ | |||
| name: Points Allocation | |||
| name: Contributor Points Tracker | |||
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This GitHub Actions workflow file is missing the required Microsoft copyright header. According to repository standards, all source files including workflow YAML files should start with a copyright header before any other content.
| except Exception as e: | ||
| print(f"ERROR: GitHub API request failed: {e}", file=sys.stderr) | ||
| return {} |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error handling in this function catches all exceptions and returns an empty dictionary, which can mask genuine errors like authentication failures or network issues. This makes debugging difficult. Consider logging the specific error type and re-raising critical errors (like authentication failures) or at least returning an error indicator so the caller knows the request failed versus legitimately returning an empty result.
| # Keywords that link issues: closes, fixes, resolves (case-insensitive) | ||
| import re | ||
| # Pattern matches: "closes #123", "fixes #456", "resolves #789", etc. | ||
| pattern = r'(?:close[sd]?|fix(?:e[sd])?|resolve[sd]?)\s*:?\s*#(\d+)' |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This regex pattern has a potential issue: it will match partial words. For example, "closed" would match both "close" and "closed", and "fixing" would match "fix". While the current pattern uses word boundaries implicitly through the grouping, it could still match unintended variations. Consider using more precise patterns like r'\b(closes?|fixe[sd]|resolved?)\s*:?\s*#(\d+)' to ensure proper word boundaries.
| pattern = r'(?:close[sd]?|fix(?:e[sd])?|resolve[sd]?)\s*:?\s*#(\d+)' | |
| pattern = r'\b(closes?|fixe[sd]|resolved?)\b\s*:?\s*#(\d+)' |
| def is_first_time_contributor(username: str, current_pr_number: int) -> bool: | ||
| """Check if the current PR is the user's first merged PR.""" | ||
| # Check if user has any other merged PRs (excluding current one) | ||
| # Limit to recent 100 PRs to avoid API rate limits and performance issues | ||
| try: | ||
| search_result = github_api_request('GET', f'pulls?state=all&creator={username}&per_page=100') | ||
| if isinstance(search_result, list): | ||
| # Get all merged PRs excluding the current one | ||
| other_merged_prs = [ | ||
| pr for pr in search_result | ||
| if pr.get('merged_at') and pr.get('number') != current_pr_number | ||
| ] | ||
| # First-time contributor if no other merged PRs exist | ||
| return len(other_merged_prs) == 0 | ||
| except Exception as e: | ||
| print(f"WARNING: Error checking first-time contributor status: {e}", file=sys.stderr) | ||
| return False |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API call could cause rate limit issues for repositories with many PRs from a single user. The function limits to 100 PRs but doesn't implement pagination or rate limit handling. Additionally, checking if a user is a first-time contributor by fetching all their PRs is inefficient. Consider using the GitHub API's contributor statistics endpoint or checking the 'author_association' field which indicates 'FIRST_TIME_CONTRIBUTOR' status.
| def is_first_time_contributor(username: str, current_pr_number: int) -> bool: | |
| """Check if the current PR is the user's first merged PR.""" | |
| # Check if user has any other merged PRs (excluding current one) | |
| # Limit to recent 100 PRs to avoid API rate limits and performance issues | |
| try: | |
| search_result = github_api_request('GET', f'pulls?state=all&creator={username}&per_page=100') | |
| if isinstance(search_result, list): | |
| # Get all merged PRs excluding the current one | |
| other_merged_prs = [ | |
| pr for pr in search_result | |
| if pr.get('merged_at') and pr.get('number') != current_pr_number | |
| ] | |
| # First-time contributor if no other merged PRs exist | |
| return len(other_merged_prs) == 0 | |
| except Exception as e: | |
| print(f"WARNING: Error checking first-time contributor status: {e}", file=sys.stderr) | |
| return False | |
| def is_first_time_contributor(pr_details: dict) -> bool: | |
| """ | |
| Check if the PR author is a first-time contributor using the 'author_association' field. | |
| Returns True if the author_association is 'FIRST_TIME_CONTRIBUTOR'. | |
| """ | |
| author_association = pr_details.get('author_association', '') | |
| return author_association.upper() == 'FIRST_TIME_CONTRIBUTOR' |
| if not has_security: | ||
| for issue in linked_issues: | ||
| issue_labels = [label['name'].lower() for label in issue.get('labels', [])] | ||
| if any('security' in label or 'vulnerability' in label for label in issue_labels): | ||
| has_security = True | ||
| breakdown_source = f'closes issue #{issue["number"]}' | ||
| break | ||
| else: | ||
| breakdown_source = 'PR labeled' | ||
|
|
||
| # Award security bonus only once | ||
| if has_security: | ||
| bonus = config['points'].get('security_fix', 15) | ||
| points += bonus | ||
| breakdown.append(f'Security fix ({breakdown_source}): +{bonus} points') |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable name 'breakdown_source' is defined inside a conditional block but may be referenced outside of it. If 'has_security' is True but was set from PR labels (lines 210-221), the variable 'breakdown_source' is assigned on line 221. However, if the logic changes or the flow is unclear, this could lead to an UnboundLocalError. Consider initializing 'breakdown_source' before the conditional blocks to ensure it's always defined when used on line 227.
| pr_number = pr_details.get('number') | ||
|
|
||
| # Keywords that link issues: closes, fixes, resolves (case-insensitive) | ||
| import re |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import statement for the 're' module is placed inside a function rather than at the top of the file with other imports. This violates PEP 8 style guidelines which recommend placing all imports at the top of the module. Move this import to line 38 with the other standard library imports.
| def aggregate_contributor_points(reviews: List[dict], comments: List[dict], pr_details: dict, config: dict) -> Dict[str, dict]: | ||
| """ | ||
| Aggregate points for all contributors on a PR. | ||
| Returns: | ||
| Dict mapping username to {'total': int, 'activities': [list of activity dicts]} | ||
| """ | ||
| contributors = {} | ||
|
|
||
| # Process reviews | ||
| for review in reviews: | ||
| username = review['user']['login'] | ||
| if username not in contributors: | ||
| contributors[username] = {'total': 0, 'activities': []} | ||
|
|
||
| points, breakdown = calculate_review_points(review, config) | ||
| contributors[username]['total'] += points | ||
| contributors[username]['activities'].append({ | ||
| 'type': 'review', | ||
| 'points': points, | ||
| 'breakdown': breakdown, | ||
| 'timestamp': review['submitted_at'], | ||
| 'state': review['state'] | ||
| }) | ||
|
|
||
| # Add PR author points (if PR is merged or has special labels) | ||
| pr_author = pr_details.get('user', {}).get('login') | ||
| if pr_author: | ||
| author_points, author_breakdown = calculate_pr_author_points(pr_details, config) | ||
| if author_points > 0: | ||
| if pr_author not in contributors: | ||
| contributors[pr_author] = {'total': 0, 'activities': []} | ||
|
|
||
| contributors[pr_author]['total'] += author_points | ||
| contributors[pr_author]['activities'].append({ | ||
| 'type': 'pr_author', | ||
| 'points': author_points, | ||
| 'breakdown': author_breakdown, | ||
| 'timestamp': pr_details.get('merged_at') or pr_details.get('created_at') | ||
| }) | ||
|
|
||
| return contributors |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function 'aggregate_contributor_points' receives a 'comments' parameter but never uses it. The function's docstring says it aggregates points from "reviews, comments, and pr_details", but the implementation only processes reviews and PR author points - not individual comments. Either implement comment processing (as documented in the module docstring on line 23) or remove the unused parameter and update the docstring.
| """Get issues linked to this PR by checking PR body for closing keywords.""" | ||
| linked_issues = [] | ||
| pr_body = pr_details.get('body', '') or '' | ||
| pr_number = pr_details.get('number') |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable pr_number is not used.
| pr_number = pr_details.get('number') |
|
|
||
| if existing_comment_id: | ||
| # Update existing comment | ||
| result = github_api_request('PATCH', f'issues/comments/{existing_comment_id}', {'body': body}) |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable result is not used.
| result = github_api_request('PATCH', f'issues/comments/{existing_comment_id}', {'body': body}) | |
| github_api_request('PATCH', f'issues/comments/{existing_comment_id}', {'body': body}) |
| print(f"✅ Updated tracking comment (ID: {existing_comment_id})") | ||
| else: | ||
| # Create new comment | ||
| result = github_api_request('POST', f'issues/{pr_number}/comments', {'body': body}) | ||
| print(f"✅ Created new tracking comment") | ||
|
|
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable result is not used.
| print(f"✅ Updated tracking comment (ID: {existing_comment_id})") | |
| else: | |
| # Create new comment | |
| result = github_api_request('POST', f'issues/{pr_number}/comments', {'body': body}) | |
| print(f"✅ Created new tracking comment") | |
| if isinstance(result, dict) and 'id' in result: | |
| print(f"✅ Updated tracking comment (ID: {result['id']})") | |
| else: | |
| print(f"⚠️ Failed to update tracking comment (ID: {existing_comment_id}) - API response: {result}") | |
| else: | |
| # Create new comment | |
| result = github_api_request('POST', f'issues/{pr_number}/comments', {'body': body}) | |
| if isinstance(result, dict) and 'id' in result: | |
| print(f"✅ Created new tracking comment (ID: {result['id']})") | |
| else: | |
| print(f"⚠️ Failed to create tracking comment - API response: {result}") |
| """Make GitHub API request.""" | ||
| url = f"https://api.github.com/repos/{GITHUB_REPOSITORY}/{endpoint}" | ||
| headers = { | ||
| 'Authorization': f'token {GITHUB_TOKEN}', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tokens are not safe if we developers run this, they can fetch the token.
Adding the points allocated to users in a PR comment