diff --git a/README.md b/README.md index 78d287f..68e80f3 100644 --- a/README.md +++ b/README.md @@ -393,9 +393,9 @@ scoring: points: commit: 10 commit_with_tests: 15 + # Line scoring always uses meaningful lines (excludes comments/whitespace) lines_added: 0.1 lines_deleted: 0.05 - use_meaningful_lines: true # Exclude comments/whitespace from line scoring pr_opened: 25 pr_merged: 50 pr_reviewed: 30 @@ -428,7 +428,6 @@ options: additional_bot_patterns: - "my-org-bot" - "jenkins*" - use_local_git: true clone_directory: "./.repos" user_aliases: - github_login: "username" @@ -481,7 +480,7 @@ options: ### Meaningful Lines Filtering -By default, Git Velocity filters out non-meaningful code changes when scoring line additions and deletions. This provides a more accurate measure of actual code contributions. +Git Velocity always filters out non-meaningful code changes when scoring line additions and deletions. This provides an accurate measure of actual code contributions. **What's filtered out:** - **Comments**: Single-line (`//`, `#`, `--`), block (`/* */`, ``), docstrings (`"""`, `'''`) @@ -496,14 +495,6 @@ By default, Git Velocity filters out non-meaningful code changes when scoring li - VB: `'` - HTML/XML: `` -To disable this filtering and score raw line counts: - -```yaml -scoring: - points: - use_meaningful_lines: false # Score all lines including comments/whitespace -``` - ### Environment Variables All configuration values support environment variable expansion: diff --git a/config.example.yaml b/config.example.yaml index 84f6f40..78cd631 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -87,10 +87,9 @@ scoring: points: commit: 10 commit_with_tests: 15 + # Line scoring always uses meaningful lines (excludes comments/whitespace) lines_added: 0.1 lines_deleted: 0.05 - # Use meaningful lines (excludes comments/whitespace) for scoring - use_meaningful_lines: true pr_opened: 25 pr_merged: 50 pr_reviewed: 30 diff --git a/docs/calculations.html b/docs/calculations.html index fe750cf..a5ec04e 100644 --- a/docs/calculations.html +++ b/docs/calculations.html @@ -335,8 +335,8 @@ Where:

- - Disable with use_meaningful_lines: false in config to use raw line counts. + + Meaningful lines filtering is always enabled to accurately reflect code contributions.

diff --git a/docs/index.html b/docs/index.html index d4ea039..c6bb241 100644 --- a/docs/index.html +++ b/docs/index.html @@ -831,7 +831,6 @@ options: concurrent_requests: 5 include_bots: false - use_local_git: true user_aliases: - github_login: "johndoe" emails: ["john@work.com", "john@personal.com"] diff --git a/internal/aggregator/aggregator.go b/internal/aggregator/aggregator.go index 670f77a..cdf7e55 100644 --- a/internal/aggregator/aggregator.go +++ b/internal/aggregator/aggregator.go @@ -365,7 +365,12 @@ func (a *Aggregator) Aggregate(data *models.RawData, dateRange *config.ParsedDat changesRequestedPRs := prChangesRequested[login] // Count merged PRs that didn't have changes requested for _, pr := range data.PullRequests { - if pr.Author.Login == login && pr.IsMerged() { + // Normalize PR author login before comparison + prLogin := pr.Author.Login + if mapped, ok := loginToLogin[prLogin]; ok { + prLogin = mapped + } + if prLogin == login && pr.IsMerged() { if changesRequestedPRs == nil || !changesRequestedPRs[pr.Number] { cm.PerfectPRs++ } @@ -437,12 +442,18 @@ func (a *Aggregator) Aggregate(data *models.RawData, dateRange *config.ParsedDat } // Count issue references in commits (e.g., "fixes #123", "closes #456", "refs #789") + // Skip merge commits which naturally contain #PR numbers for _, commit := range data.Commits { login := commit.Author.Login if login == "" { continue } + // Skip merge commits - they contain #PR numbers that shouldn't count as issue refs + if isMergeCommit(commit.Message) { + continue + } + // Normalize login if mappedLogin, ok := emailToLogin[commit.Author.Email]; ok { login = mappedLogin @@ -465,6 +476,22 @@ func (a *Aggregator) Aggregate(data *models.RawData, dateRange *config.ParsedDat } } + // Build reverse mapping: raw PR author login -> normalized login + // This is needed because contributorMap keys are normalized but pr.Author.Login is not + prAuthorToNormalizedLogin := make(map[string]string) + for _, pr := range data.PullRequests { + rawLogin := pr.Author.Login + if rawLogin == "" { + continue + } + normalizedLogin := rawLogin + // Check if this raw login maps to a different normalized login + if mapped, ok := loginToLogin[rawLogin]; ok { + normalizedLogin = mapped + } + prAuthorToNormalizedLogin[rawLogin] = normalizedLogin + } + // Calculate averages and finalize contributor metrics for login, cm := range contributorMap { // Calculate average time to merge @@ -481,7 +508,12 @@ func (a *Aggregator) Aggregate(data *models.RawData, dateRange *config.ParsedDat if cm.PRsOpened > 0 { totalPRLines := 0 for _, pr := range data.PullRequests { - if pr.Author.Login == login { + // Normalize PR author login before comparison + prLogin := pr.Author.Login + if normalized, ok := prAuthorToNormalizedLogin[prLogin]; ok { + prLogin = normalized + } + if prLogin == login { totalPRLines += pr.TotalChanges() } } @@ -531,7 +563,12 @@ func (a *Aggregator) Aggregate(data *models.RawData, dateRange *config.ParsedDat if rcm.PRsOpened > 0 { totalPRLines := 0 for _, pr := range data.PullRequests { - if pr.Author.Login == login && pr.Repository == repo { + // Normalize PR author login before comparison + prLogin := pr.Author.Login + if mapped, ok := loginToLogin[prLogin]; ok { + prLogin = mapped + } + if prLogin == login && pr.Repository == repo { totalPRLines += pr.TotalChanges() } } @@ -540,7 +577,12 @@ func (a *Aggregator) Aggregate(data *models.RawData, dateRange *config.ParsedDat // Calculate perfect PRs for this repo for _, pr := range data.PullRequests { - if pr.Author.Login == login && pr.Repository == repo && pr.IsMerged() { + // Normalize PR author login before comparison + prLogin := pr.Author.Login + if mapped, ok := loginToLogin[prLogin]; ok { + prLogin = mapped + } + if prLogin == login && pr.Repository == repo && pr.IsMerged() { changesRequestedPRs := prChangesRequested[login] if changesRequestedPRs == nil || !changesRequestedPRs[pr.Number] { rcm.PerfectPRs++ @@ -1332,8 +1374,10 @@ func calculateStreaks(days map[string]bool) (longest, current int) { streak := 1 for i := 1; i < len(dates); i++ { - diff := dates[i].Sub(dates[i-1]).Hours() / 24 - if diff == 1 { + // Use integer day difference to avoid floating point precision issues with DST + diffHours := dates[i].Sub(dates[i-1]).Hours() + diffDays := int(diffHours/24 + 0.5) // Round to nearest integer + if diffDays == 1 { streak++ if streak > longest { longest = streak @@ -1345,8 +1389,10 @@ func calculateStreaks(days map[string]bool) (longest, current int) { // Check if current streak is still active (last activity was today or yesterday) today := time.Now().Truncate(24 * time.Hour) - lastActive := dates[len(dates)-1] - daysSinceLastActive := today.Sub(lastActive).Hours() / 24 + // Truncate lastActive to midnight as well for consistent comparison + lastActive := dates[len(dates)-1].Truncate(24 * time.Hour) + diffHours := today.Sub(lastActive).Hours() + daysSinceLastActive := int(diffHours/24 + 0.5) // Round to nearest integer if daysSinceLastActive <= 1 { current = streak @@ -1385,3 +1431,25 @@ func countIssueReferences(message string) int { return count } + +// isMergeCommit checks if a commit message indicates a merge commit +// Merge commits should be skipped when counting issue references as they +// naturally contain #PR numbers from the merged PR titles +func isMergeCommit(message string) bool { + // Common merge commit patterns: + // - "Merge pull request #123 from ..." + // - "Merge branch 'feature' into ..." + // - "Merge remote-tracking branch ..." + // - "Merge commit ..." + if len(message) < 6 { + return false + } + + // Check if message starts with "Merge " (case-insensitive for first letter) + prefix := message[:6] + if prefix == "Merge " || prefix == "merge " { + return true + } + + return false +} diff --git a/internal/aggregator/streak_calculation_bugs_test.go b/internal/aggregator/streak_calculation_bugs_test.go new file mode 100644 index 0000000..ee2124d --- /dev/null +++ b/internal/aggregator/streak_calculation_bugs_test.go @@ -0,0 +1,220 @@ +package aggregator + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" +) + +// TestStreakCalculation_FloatPrecisionBug tests the potential floating point precision issues in streak calculation +func TestStreakCalculation_FloatPrecisionBug(t *testing.T) { + t.Parallel() + + t.Run("consecutive days with different hours", func(t *testing.T) { + t.Parallel() + + // Bug: Line 1335 in aggregator.go uses floating point division + // diff := dates[i].Sub(dates[i-1]).Hours() / 24 + // This can cause precision issues when checking diff == 1 + dates := map[string]bool{ + "2024-01-15": true, // Day 1 at 00:00 + "2024-01-16": true, // Day 2 at 00:00 + "2024-01-17": true, // Day 3 at 00:00 + } + + longest, _ := calculateStreaks(dates) + + // This should be 3, but floating point comparison might fail + assert.Equal(t, 3, longest, "Should calculate 3-day streak correctly") + }) + + t.Run("dates with daylight saving time boundary", func(t *testing.T) { + t.Parallel() + + // Create dates that cross a DST boundary + // On DST change, a "day" might be 23 or 25 hours, not exactly 24 + // This would cause the streak to break incorrectly + loc, _ := time.LoadLocation("America/New_York") + + // March 2024: DST starts on March 10, 2024 at 2:00 AM (clocks move to 3:00 AM) + day1 := time.Date(2024, 3, 9, 12, 0, 0, 0, loc) // Day before DST + day2 := time.Date(2024, 3, 10, 12, 0, 0, 0, loc) // DST change day (23-hour day) + day3 := time.Date(2024, 3, 11, 12, 0, 0, 0, loc) // Day after DST + + dates := map[string]bool{ + day1.Format("2006-01-02"): true, + day2.Format("2006-01-02"): true, + day3.Format("2006-01-02"): true, + } + + longest, _ := calculateStreaks(dates) + + // Bug: The floating point comparison diff == 1 might fail due to DST + // day1 to day2: 23 hours / 24 = 0.958... != 1.0 (streak breaks) + // This test documents the bug - it should pass with value 3, but might return 1 or 2 + assert.GreaterOrEqual(t, longest, 1, "Should handle DST boundaries") + // The actual expected value is 3, but due to the bug it might be less + }) + + t.Run("consecutive days at different times of day", func(t *testing.T) { + t.Parallel() + + // Even without DST, different times of day can cause issues + // Day 1 at 10:00, Day 2 at 9:00 = 23 hours apart (not exactly 24) + // 23 / 24 = 0.958... != 1.0 + loc := time.UTC + day1 := time.Date(2024, 1, 15, 10, 0, 0, 0, loc) + day2 := time.Date(2024, 1, 16, 9, 0, 0, 0, loc) // 23 hours later + day3 := time.Date(2024, 1, 17, 11, 0, 0, 0, loc) // 26 hours later + + dates := map[string]bool{ + day1.Format("2006-01-02"): true, + day2.Format("2006-01-02"): true, + day3.Format("2006-01-02"): true, + } + + longest, _ := calculateStreaks(dates) + + // With float comparison, this might break the streak + // Expected: 3, Actual might be: 1, 2, or 3 depending on precision + assert.GreaterOrEqual(t, longest, 1, "Should not panic") + // Document: This is a known bug - should be 3 but might be less due to time differences + }) +} + +// TestStreakCalculation_CurrentStreakBoundaryCondition tests current streak calculation edge cases +func TestStreakCalculation_CurrentStreakBoundaryCondition(t *testing.T) { + t.Parallel() + + t.Run("last activity exactly 1 day ago", func(t *testing.T) { + t.Parallel() + + // Line 1351: if daysSinceLastActive <= 1 + // This uses float comparison which can be problematic + now := time.Now() + yesterday := now.Add(-24 * time.Hour) + + dates := map[string]bool{ + yesterday.Format("2006-01-02"): true, + } + + _, current := calculateStreaks(dates) + + // Float comparison: (now - yesterday).Hours() / 24 might not be exactly 1.0 + // Due to precision, it might be 0.999... or 1.001... + // This test should pass but documents the fragility + assert.GreaterOrEqual(t, current, 0, "Should not panic") + }) + + t.Run("last activity exactly at boundary", func(t *testing.T) { + t.Parallel() + + // Edge case: What if the last activity was exactly 24.0000 hours ago? + // Line 1351: daysSinceLastActive <= 1 + // With float precision, 24.0 hours / 24 = 1.0, so <= 1 should pass + now := time.Now().Truncate(24 * time.Hour) + exactlyOneDayAgo := now.Add(-24 * time.Hour) + + dates := map[string]bool{ + exactlyOneDayAgo.Format("2006-01-02"): true, + } + + _, current := calculateStreaks(dates) + + // This should preserve the streak since it's exactly 1 day + // But float precision might cause issues + assert.GreaterOrEqual(t, current, 0, "Should handle exact 24-hour boundary") + }) +} + +// TestStreakCalculation_EmptyOrSingleDate tests edge cases with minimal data +func TestStreakCalculation_EmptyOrSingleDate(t *testing.T) { + t.Parallel() + + t.Run("empty dates map", func(t *testing.T) { + t.Parallel() + + dates := map[string]bool{} + longest, current := calculateStreaks(dates) + + assert.Equal(t, 0, longest) + assert.Equal(t, 0, current) + }) + + t.Run("single date", func(t *testing.T) { + t.Parallel() + + dates := map[string]bool{ + "2024-01-15": true, + } + + longest, current := calculateStreaks(dates) + + assert.Equal(t, 1, longest, "Single date should be streak of 1") + // current depends on how far in the past this date is + assert.GreaterOrEqual(t, current, 0) + }) +} + +// TestStreakCalculation_DateParsingError documents behavior with invalid dates +func TestStreakCalculation_DateParsingError(t *testing.T) { + t.Parallel() + + t.Run("invalid date format", func(t *testing.T) { + t.Parallel() + + dates := map[string]bool{ + "invalid-date": true, + "2024-01-15": true, + } + + // The function parses dates with time.Parse("2006-01-02", dateStr) + // Invalid dates are silently skipped (err != nil check on line 1316) + longest, current := calculateStreaks(dates) + + // Only the valid date counts + assert.Equal(t, 1, longest, "Should skip invalid dates") + assert.GreaterOrEqual(t, current, 0) + }) +} + +// TestStreakCalculation_LargeGaps tests streak reset with large gaps +func TestStreakCalculation_LargeGaps(t *testing.T) { + t.Parallel() + + t.Run("large gap between dates", func(t *testing.T) { + t.Parallel() + + dates := map[string]bool{ + "2024-01-01": true, + "2024-01-02": true, + "2024-01-03": true, + "2024-02-15": true, // Large gap - should reset streak + "2024-02-16": true, + } + + longest, _ := calculateStreaks(dates) + + // Longest streak should be 3 (Jan 1-3) + assert.Equal(t, 3, longest, "Should correctly identify longest streak despite gap") + }) + + t.Run("multiple equal-length streaks", func(t *testing.T) { + t.Parallel() + + dates := map[string]bool{ + "2024-01-01": true, + "2024-01-02": true, + "2024-01-03": true, + "2024-02-01": true, // Gap + "2024-02-02": true, + "2024-02-03": true, + } + + longest, _ := calculateStreaks(dates) + + // Two 3-day streaks - should return 3 + assert.Equal(t, 3, longest, "Should return longest streak when multiple equal streaks exist") + }) +} diff --git a/internal/app/app.go b/internal/app/app.go index ead7dbe..6af26f7 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -58,18 +58,16 @@ func (a *App) Run(ctx context.Context) error { a.log("%s", msg) }) - // Initialize local git repository manager if using local git - if a.config.Options.UseLocalGit { - a.log("Initializing local git repository manager...") - gitRepo, err := git.NewRepository(a.config.Options.CloneDirectory) - if err != nil { - return fmt.Errorf("failed to create git repository manager: %w", err) - } - gitRepo.SetProgressCallback(func(msg string) { - a.log("%s", msg) - }) - a.gitRepo = gitRepo + // Initialize local git repository manager (always used for accurate commit data) + a.log("Initializing local git repository manager...") + gitRepo, err := git.NewRepository(a.config.Options.CloneDirectory) + if err != nil { + return fmt.Errorf("failed to create git repository manager: %w", err) } + gitRepo.SetProgressCallback(func(msg string) { + a.log("%s", msg) + }) + a.gitRepo = gitRepo // Parse date range dateRange, err := a.config.GetParsedDateRange() @@ -163,44 +161,31 @@ func (a *App) collectRepoData(ctx context.Context, owner, name string, dateRange repoName := fmt.Sprintf("%s/%s", owner, name) a.log(" Fetching data from %s...", repoName) - // Fetch commits - use local git if enabled (much faster) - var commits []models.Commit - var err error + // Clone/update repository locally (required for accurate commit data) + token := a.config.Auth.GithubToken - if a.gitRepo != nil { - // Clone/update repository locally - token := a.config.Auth.GithubToken - - // Determine clone options (shallow clone if enabled) - var cloneOpts *git.CloneOptions - if a.config.Options.ShallowClone && dateRange.Start != nil { - // Get commit count since start date to determine shallow clone depth - commitCount, countErr := a.client.GetCommitCountSince(ctx, owner, name, *dateRange.Start) - if countErr != nil { - a.log(" Warning: failed to get commit count for shallow clone: %v", countErr) - // Proceed with full clone - } else if commitCount > 0 { - // Add buffer for safety margin - depth := commitCount + a.config.Options.ShallowCloneBuffer - cloneOpts = &git.CloneOptions{Depth: depth} - a.log(" Using shallow clone (depth: %d = %d commits + %d buffer)", depth, commitCount, a.config.Options.ShallowCloneBuffer) - } + // Determine clone options (shallow clone if enabled) + var cloneOpts *git.CloneOptions + if a.config.Options.ShallowClone && dateRange.Start != nil { + // Get commit count since start date to determine shallow clone depth + commitCount, countErr := a.client.GetCommitCountSince(ctx, owner, name, *dateRange.Start) + if countErr != nil { + a.log(" Warning: failed to get commit count for shallow clone: %v", countErr) + // Proceed with full clone + } else if commitCount > 0 { + // Add buffer for safety margin + depth := commitCount + a.config.Options.ShallowCloneBuffer + cloneOpts = &git.CloneOptions{Depth: depth} + a.log(" Using shallow clone (depth: %d = %d commits + %d buffer)", depth, commitCount, a.config.Options.ShallowCloneBuffer) } - - cloneErr := a.gitRepo.EnsureClonedWithOptions(ctx, owner, name, token, cloneOpts) - if cloneErr != nil { - a.log(" Warning: failed to clone repository locally, falling back to API: %v", cloneErr) - // Fallback to API - commits, err = a.client.FetchCommits(ctx, owner, name, dateRange.Start, dateRange.End) - } else { - // Use local git for commits - commits, err = a.gitRepo.FetchCommits(ctx, owner, name, dateRange.Start, dateRange.End) - } - } else { - // Use API for commits - commits, err = a.client.FetchCommits(ctx, owner, name, dateRange.Start, dateRange.End) } + if err := a.gitRepo.EnsureClonedWithOptions(ctx, owner, name, token, cloneOpts); err != nil { + return fmt.Errorf("failed to clone repository %s: %w", repoName, err) + } + + // Fetch commits from local git clone + commits, err := a.gitRepo.FetchCommits(ctx, owner, name, dateRange.Start, dateRange.End) if err != nil { return fmt.Errorf("failed to fetch commits: %w", err) } @@ -238,9 +223,21 @@ func (a *App) collectRepoData(ctx context.Context, owner, name string, dateRange } } else { // Use REST API - if _, _, err := a.fetchPRsAndReviewsREST(ctx, owner, name, dateRange, data); err != nil { + prs, reviews, err := a.fetchPRsAndReviewsREST(ctx, owner, name, dateRange, data) + if err != nil { return err } + // Filter out bots and add to data + for _, pr := range prs { + if !a.config.IsBot(pr.Author.Login) { + data.PullRequests = append(data.PullRequests, pr) + } + } + for _, r := range reviews { + if !a.config.IsBot(r.Author.Login) { + data.Reviews = append(data.Reviews, r) + } + } } // Fetch issues and comments diff --git a/internal/config/schema.go b/internal/config/schema.go index 4e8c816..bca4fab 100644 --- a/internal/config/schema.go +++ b/internal/config/schema.go @@ -90,10 +90,6 @@ type PointsConfig struct { FastReview4h int `yaml:"fast_review_4h"` FastReview24h int `yaml:"fast_review_24h"` OutOfHours int `yaml:"out_of_hours"` // Bonus per commit outside 9am-5pm - - // UseMeaningfulLines determines whether scoring uses meaningful lines (excluding comments/whitespace) - // or raw line counts. Default is true for more accurate contribution scoring. - UseMeaningfulLines bool `yaml:"use_meaningful_lines"` } // AchievementConfig defines an achievement badge @@ -149,7 +145,6 @@ type OptionsConfig struct { IncludeBots bool `yaml:"include_bots"` AdditionalBotPatterns []string `yaml:"additional_bot_patterns"` // User-defined patterns (added to hardcoded defaults) CloneDirectory string `yaml:"clone_directory"` // Directory for local git clones - UseLocalGit bool `yaml:"use_local_git"` // Use local git for commits (faster) ShallowClone bool `yaml:"shallow_clone"` // Use shallow clone based on date range (faster cloning) ShallowCloneBuffer int `yaml:"shallow_clone_buffer"` // Extra commits to fetch beyond date range (default: 100) UseGraphQL bool `yaml:"use_graphql"` // Use GraphQL API for batched queries (fewer API calls) @@ -196,23 +191,22 @@ func DefaultConfig() *Config { Scoring: ScoringConfig{ Enabled: true, Points: PointsConfig{ - Commit: 10, - CommitWithTests: 15, - LinesAdded: 0.1, - LinesDeleted: 0.05, - PROpened: 25, - PRMerged: 50, - PRReviewed: 30, - ReviewComment: 5, - IssueOpened: 10, - IssueClosed: 20, - IssueComment: 5, - IssueReference: 5, - FastReview1h: 50, - FastReview4h: 25, - FastReview24h: 10, - OutOfHours: 2, - UseMeaningfulLines: true, // Default to meaningful lines for accurate contribution scoring + Commit: 10, + CommitWithTests: 15, + LinesAdded: 0.1, + LinesDeleted: 0.05, + PROpened: 25, + PRMerged: 50, + PRReviewed: 30, + ReviewComment: 5, + IssueOpened: 10, + IssueClosed: 20, + IssueComment: 5, + IssueReference: 5, + FastReview1h: 50, + FastReview4h: 25, + FastReview24h: 10, + OutOfHours: 2, }, }, Output: OutputConfig{ @@ -233,7 +227,6 @@ func DefaultConfig() *Config { IncludeBots: false, AdditionalBotPatterns: []string{}, // Users can add custom patterns here CloneDirectory: "./.repos", - UseLocalGit: true, // Default to faster local git analysis ShallowClone: true, // Default to shallow clone for faster cloning ShallowCloneBuffer: 25, // Extra commits beyond date range for safety margin UseGraphQL: true, // Default to GraphQL for fewer API calls diff --git a/internal/domain/scoring/calculator.go b/internal/domain/scoring/calculator.go index 7336185..d198440 100644 --- a/internal/domain/scoring/calculator.go +++ b/internal/domain/scoring/calculator.go @@ -80,10 +80,15 @@ func (c *Calculator) Calculate(metrics *models.GlobalMetrics) *models.GlobalMetr return contributors[i].Score.Total > contributors[j].Score.Total }) - // Assign ranks + // Assign ranks (guard against empty slice for percentile calculation) + numContributors := len(contributors) for i := range contributors { contributors[i].Score.Rank = i + 1 - contributors[i].Score.PercentileRank = float64(len(contributors)-i) / float64(len(contributors)) * 100 + if numContributors > 0 { + contributors[i].Score.PercentileRank = float64(numContributors-i) / float64(numContributors) * 100 + } else { + contributors[i].Score.PercentileRank = 0 + } } // Build leaderboard @@ -167,15 +172,10 @@ func (c *Calculator) calculateScore(cm *models.ContributorMetrics) models.Score // Commit points breakdown.Commits = cm.CommitCount * points.Commit - // Line change points - use meaningful lines if configured, otherwise raw counts - linesAdded := cm.LinesAdded - linesDeleted := cm.LinesDeleted - if points.UseMeaningfulLines { - linesAdded = cm.MeaningfulLinesAdded - linesDeleted = cm.MeaningfulLinesDeleted - } - breakdown.LineChanges = int(float64(linesAdded)*points.LinesAdded + - float64(linesDeleted)*points.LinesDeleted) + // Line change points - always use meaningful lines (excluding comments/whitespace) + // to accurately reflect actual code contribution + breakdown.LineChanges = int(float64(cm.MeaningfulLinesAdded)*points.LinesAdded + + float64(cm.MeaningfulLinesDeleted)*points.LinesDeleted) // PR points breakdown.PRs = cm.PRsOpened*points.PROpened + cm.PRsMerged*points.PRMerged diff --git a/internal/domain/scoring/calculator_test.go b/internal/domain/scoring/calculator_test.go index 472866f..576dfeb 100644 --- a/internal/domain/scoring/calculator_test.go +++ b/internal/domain/scoring/calculator_test.go @@ -71,6 +71,8 @@ func TestCalculator_BasicScoring(t *testing.T) { CommitCount: 10, LinesAdded: 1000, LinesDeleted: 500, + MeaningfulLinesAdded: 1000, // Same as raw for this test + MeaningfulLinesDeleted: 500, PRsOpened: 5, PRsMerged: 3, ReviewsGiven: 8, @@ -91,7 +93,7 @@ func TestCalculator_BasicScoring(t *testing.T) { // Verify score breakdown: // Commits: 10 * 10 = 100 - // Lines: 1000 * 0.1 + 500 * 0.05 = 100 + 25 = 125 + // Lines (meaningful): 1000 * 0.1 + 500 * 0.05 = 100 + 25 = 125 // PRs: 5 * 25 + 3 * 50 = 125 + 150 = 275 // Reviews: 8 * 30 + 20 * 5 = 240 + 100 = 340 // Total: 100 + 125 + 275 + 340 = 840 @@ -860,10 +862,9 @@ func TestCalculator_MeaningfulLinesScoring(t *testing.T) { cfg := config.DefaultConfig() cfg.Scoring.Enabled = true cfg.Scoring.Points = config.PointsConfig{ - Commit: 10, - LinesAdded: 0.1, - LinesDeleted: 0.05, - UseMeaningfulLines: true, // Use meaningful lines + Commit: 10, + LinesAdded: 0.1, + LinesDeleted: 0.05, } calc := NewCalculator(cfg) @@ -897,58 +898,15 @@ func TestCalculator_MeaningfulLinesScoring(t *testing.T) { assert.Equal(t, 200, contributor.Score.Total) }) - t.Run("uses raw lines when disabled", func(t *testing.T) { - t.Parallel() - - cfg := config.DefaultConfig() - cfg.Scoring.Enabled = true - cfg.Scoring.Points = config.PointsConfig{ - Commit: 10, - LinesAdded: 0.1, - LinesDeleted: 0.05, - UseMeaningfulLines: false, // Use raw lines - } - calc := NewCalculator(cfg) - - metrics := &models.GlobalMetrics{ - Repositories: []models.RepositoryMetrics{ - { - FullName: "owner/repo", - Contributors: []models.ContributorMetrics{ - { - Login: "user1", - CommitCount: 10, - LinesAdded: 1000, // Raw lines - LinesDeleted: 500, - MeaningfulLinesAdded: 800, // Meaningful lines (should be ignored) - MeaningfulLinesDeleted: 400, - RepositoriesContributed: []string{"owner/repo"}, - }, - }, - }, - }, - } - - result := calc.Calculate(metrics) - - contributor := result.Repositories[0].Contributors[0] - // Line change points should use raw lines: - // Raw: 1000 * 0.1 + 500 * 0.05 = 100 + 25 = 125 - assert.Equal(t, 125, contributor.Score.Breakdown.LineChanges) - // Total: Commits (10 * 10 = 100) + Lines (125) = 225 - assert.Equal(t, 225, contributor.Score.Total) - }) - t.Run("comment-only changes score zero meaningful lines", func(t *testing.T) { t.Parallel() cfg := config.DefaultConfig() cfg.Scoring.Enabled = true cfg.Scoring.Points = config.PointsConfig{ - Commit: 10, - LinesAdded: 0.1, - LinesDeleted: 0.05, - UseMeaningfulLines: true, + Commit: 10, + LinesAdded: 0.1, + LinesDeleted: 0.05, } calc := NewCalculator(cfg) diff --git a/internal/github/cache/cache.go b/internal/github/cache/cache.go index eb14229..d12a121 100644 --- a/internal/github/cache/cache.go +++ b/internal/github/cache/cache.go @@ -165,20 +165,28 @@ func NewMemoryCache(ttl time.Duration) *MemoryCache { // Get retrieves a value from the cache func (c *MemoryCache) Get(key string) (interface{}, bool) { c.mu.RLock() - defer c.mu.RUnlock() - entry, ok := c.data[key] if !ok { + c.mu.RUnlock() return nil, false } - // Check expiration + // Check expiration - if expired, upgrade to write lock to delete if time.Now().After(entry.ExpiresAt) { - delete(c.data, key) + c.mu.RUnlock() + // Upgrade to write lock for deletion + c.mu.Lock() + // Re-check in case another goroutine already deleted it + if entry, ok := c.data[key]; ok && time.Now().After(entry.ExpiresAt) { + delete(c.data, key) + } + c.mu.Unlock() return nil, false } - return entry.Value, true + value := entry.Value + c.mu.RUnlock() + return value, true } // Set stores a value in the cache diff --git a/internal/github/graphql.go b/internal/github/graphql.go index b5b3a28..5f6a3cd 100644 --- a/internal/github/graphql.go +++ b/internal/github/graphql.go @@ -39,9 +39,15 @@ func newProgressBar(label string, total int) *progressBar { func (p *progressBar) update(fetched int) { p.current = fetched - percent := float64(p.current) / float64(p.total) - if percent > 1.0 { - percent = 1.0 + // Guard against division by zero + var percent float64 + if p.total > 0 { + percent = float64(p.current) / float64(p.total) + if percent > 1.0 { + percent = 1.0 + } + } else { + percent = 0.0 } labelStyle := lipgloss.NewStyle().Foreground(lipgloss.Color("205"))