added custom badge theme colors cli-flags#29
Merged
Conversation
📝 WalkthroughWalkthroughAdds nine CLI flags for per-color badge overrides, wires them into configuration, validates hex values, populates an optional CustomColors in BadgeOptions, and applies those overrides during SVG rendering when provided. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as "CLI Flags"
participant Config as "BadgeConfig"
participant Generator as "Badge Generator"
participant BadgeOpts as "BadgeOptions"
participant Renderer as "RenderSVG"
User->>CLI: provide --badge-color-* flags
CLI->>Config: parse flags into color fields
Config->>Generator: pass BadgeConfig
Generator->>Generator: validate hex values
alt any color set
Generator->>BadgeOpts: build CustomColors and set on options
else no colors set
Generator->>BadgeOpts: leave CustomColors nil
end
BadgeOpts->>Renderer: invoke RenderSVG with options
Renderer->>Renderer: load theme colors
alt CustomColors non-nil
Renderer->>Renderer: override theme with non-empty custom fields
end
Renderer->>Renderer: render final SVG
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cmd/gh-oss-stats/badgeGeneration.go (1)
11-34: Pre-existing:os.Exit(1)in function returningerrorundermines testability.The function signature returns
error, but validation failures callos.Exit(1)instead of returning the error. This makes error paths untestable and the return type misleading.Consider returning errors and letting the caller handle exit:
♻️ Proposed refactor
func createBadgeOptions(conf BadgeConfig) (badge.BadgeOptions, error) { badgeStyle, err := badge.BadgeStyleFromName(conf.style) if err != nil { - fmt.Fprint(os.Stderr, err.Error()) - os.Exit(1) + return badge.BadgeOptions{}, fmt.Errorf("invalid badge style: %w", err) } badgeVariant, err := badge.BadgeVariantFromName(conf.variant) if err != nil { - fmt.Fprint(os.Stderr, err.Error()) - os.Exit(1) + return badge.BadgeOptions{}, fmt.Errorf("invalid badge variant: %w", err) } badgeTheme, err := badge.BadgeThemeFromName(conf.theme) if err != nil { - fmt.Fprint(os.Stderr, err.Error()) - os.Exit(1) + return badge.BadgeOptions{}, fmt.Errorf("invalid badge theme: %w", err) } badgeSortBy, err := badge.SortByFromName(conf.sort) if err != nil { - fmt.Fprint(os.Stderr, err.Error()) - os.Exit(1) + return badge.BadgeOptions{}, fmt.Errorf("invalid badge sort: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/gh-oss-stats/badgeGeneration.go` around lines 11 - 34, The function createBadgeOptions currently calls os.Exit(1) on validation failures which prevents errors from being returned; update createBadgeOptions to stop calling os.Exit and instead return the encountered errors (wrap them with context where helpful) for each call to badge.BadgeStyleFromName, badge.BadgeVariantFromName, badge.BadgeThemeFromName, and badge.SortByFromName; remove the fmt.Fprint(os.Stderr, ...) side effects and ensure the function returns a non-nil error when any lookup fails so the caller can handle logging/exiting.cmd/gh-oss-stats/badgeConfig.go (1)
47-55: Consider validating hex color format.The flags accept any string but document hex format (e.g.,
#1a1b26). Invalid values will pass through to the SVG template without validation, potentially producing malformed output.You could add validation either here during flag parsing or in
createBadgeOptions. This is optional since SVG renderers typically handle invalid colors gracefully.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/gh-oss-stats/badgeConfig.go` around lines 47 - 55, The badge color flags (fs.StringVar bindings to bf.colorBackground, bf.colorBackgroundAlt, bf.colorText, bf.colorTextSecondary, bf.colorBorder, bf.colorAccent, bf.colorPositive, bf.colorNegative, bf.colorStar) accept arbitrary strings — add hex color validation (e.g. match ^#([0-9A-Fa-f]{3}|[0-9A-Fa-f]{6})$) either right after flag parsing in the badgeConfig code or inside createBadgeOptions; on invalid values return a clear error (or reject the flag) or normalize/ignore the value so only valid hex colors reach the SVG template.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/gh-oss-stats/badgeGeneration_test.go`:
- Around line 101-113: The test duplicates the Accent assertion for
opts.CustomColors; remove the second Accent check and instead assert a different
unset color field (e.g., verify opts.CustomColors.Text == "" or
opts.CustomColors.Border == "") to improve coverage—update the failing assertion
block that currently references opts.CustomColors.Accent to check the chosen
unset field (keeping the comment about RenderSVG theme override logic).
---
Nitpick comments:
In `@cmd/gh-oss-stats/badgeConfig.go`:
- Around line 47-55: The badge color flags (fs.StringVar bindings to
bf.colorBackground, bf.colorBackgroundAlt, bf.colorText, bf.colorTextSecondary,
bf.colorBorder, bf.colorAccent, bf.colorPositive, bf.colorNegative,
bf.colorStar) accept arbitrary strings — add hex color validation (e.g. match
^#([0-9A-Fa-f]{3}|[0-9A-Fa-f]{6})$) either right after flag parsing in the
badgeConfig code or inside createBadgeOptions; on invalid values return a clear
error (or reject the flag) or normalize/ignore the value so only valid hex
colors reach the SVG template.
In `@cmd/gh-oss-stats/badgeGeneration.go`:
- Around line 11-34: The function createBadgeOptions currently calls os.Exit(1)
on validation failures which prevents errors from being returned; update
createBadgeOptions to stop calling os.Exit and instead return the encountered
errors (wrap them with context where helpful) for each call to
badge.BadgeStyleFromName, badge.BadgeVariantFromName, badge.BadgeThemeFromName,
and badge.SortByFromName; remove the fmt.Fprint(os.Stderr, ...) side effects and
ensure the function returns a non-nil error when any lookup fails so the caller
can handle logging/exiting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 49aa3b6d-cacb-4c2e-a091-b6f055afbcd5
📒 Files selected for processing (7)
cmd/gh-oss-stats/badgeConfig.gocmd/gh-oss-stats/badgeConfig_test.gocmd/gh-oss-stats/badgeGeneration.gocmd/gh-oss-stats/badgeGeneration_test.godocs/TECHNICAL.mdpkg/ossstats/badge/badge.gopkg/ossstats/badge/types.go
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/gh-oss-stats/badgeGeneration.go (1)
21-44:⚠️ Potential issue | 🟠 MajorInconsistent error handling:
os.Exitvs returning error.The function signature returns
error, and the new color validation (lines 60-66) properly returns errors. However, lines 24-26, 30-32, 36-38, and 42-44 callos.Exit(1)directly, which is inconsistent and makes the function difficult to test. As per coding guidelines, avoid usingos.Exitin library code—return descriptive errors instead.🔧 Proposed fix to return errors consistently
func createBadgeOptions(conf BadgeConfig) (badge.BadgeOptions, error) { badgeStyle, err := badge.BadgeStyleFromName(conf.style) if err != nil { - fmt.Fprint(os.Stderr, err.Error()) - os.Exit(1) + return badge.BadgeOptions{}, fmt.Errorf("invalid badge style: %w", err) } badgeVariant, err := badge.BadgeVariantFromName(conf.variant) if err != nil { - fmt.Fprint(os.Stderr, err.Error()) - os.Exit(1) + return badge.BadgeOptions{}, fmt.Errorf("invalid badge variant: %w", err) } badgeTheme, err := badge.BadgeThemeFromName(conf.theme) if err != nil { - fmt.Fprint(os.Stderr, err.Error()) - os.Exit(1) + return badge.BadgeOptions{}, fmt.Errorf("invalid badge theme: %w", err) } badgeSortBy, err := badge.SortByFromName(conf.sort) if err != nil { - fmt.Fprint(os.Stderr, err.Error()) - os.Exit(1) + return badge.BadgeOptions{}, fmt.Errorf("invalid sort option: %w", err) }As per coding guidelines: "Always return descriptive errors instead of zero values on error; do not use
log.Fataloros.Exitin library code."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/gh-oss-stats/badgeGeneration.go` around lines 21 - 44, The createBadgeOptions function currently prints errors to stderr and calls os.Exit on failures from badge.BadgeStyleFromName, badge.BadgeVariantFromName, badge.BadgeThemeFromName and badge.SortByFromName; instead, remove the fmt.Fprint/os.Exit calls and return descriptive errors from createBadgeOptions (wrap or return the err from each call with context like "invalid style", "invalid variant", "invalid theme", "invalid sort") so the function consistently returns an error (matching the color validation at lines ~60-66) and becomes testable; update the branches handling badgeStyle, badgeVariant, badgeTheme and badgeSortBy accordingly.
🧹 Nitpick comments (1)
cmd/gh-oss-stats/badgeGeneration_test.go (1)
324-334: Usestrings.Containsinstead of custom helper.The custom
containsfunction reimplementsstrings.Containsfrom the standard library. Prefer the stdlib version for clarity and maintainability.♻️ Proposed fix
import ( "testing" + "strings" "github.com/mabd-dev/gh-oss-stats/pkg/ossstats/badge" )Then replace usage:
-func contains(s, sub string) bool { - return len(s) >= len(sub) && (s == sub || len(sub) == 0 || - func() bool { - for i := 0; i <= len(s)-len(sub); i++ { - if s[i:i+len(sub)] == sub { - return true - } - } - return false - }()) -}And update calls from
contains(msg, "--badge-color-accent")tostrings.Contains(msg, "--badge-color-accent").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/gh-oss-stats/badgeGeneration_test.go` around lines 324 - 334, The custom contains function in badgeGeneration_test.go (function name contains) duplicates stdlib behavior; remove this helper and replace its uses with strings.Contains from the standard library: import "strings" in the test file if not already present, delete the contains function definition, and update all calls like contains(msg, "--badge-color-accent") to strings.Contains(msg, "--badge-color-accent") to use the canonical implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cmd/gh-oss-stats/badgeGeneration.go`:
- Around line 21-44: The createBadgeOptions function currently prints errors to
stderr and calls os.Exit on failures from badge.BadgeStyleFromName,
badge.BadgeVariantFromName, badge.BadgeThemeFromName and badge.SortByFromName;
instead, remove the fmt.Fprint/os.Exit calls and return descriptive errors from
createBadgeOptions (wrap or return the err from each call with context like
"invalid style", "invalid variant", "invalid theme", "invalid sort") so the
function consistently returns an error (matching the color validation at lines
~60-66) and becomes testable; update the branches handling badgeStyle,
badgeVariant, badgeTheme and badgeSortBy accordingly.
---
Nitpick comments:
In `@cmd/gh-oss-stats/badgeGeneration_test.go`:
- Around line 324-334: The custom contains function in badgeGeneration_test.go
(function name contains) duplicates stdlib behavior; remove this helper and
replace its uses with strings.Contains from the standard library: import
"strings" in the test file if not already present, delete the contains function
definition, and update all calls like contains(msg, "--badge-color-accent") to
strings.Contains(msg, "--badge-color-accent") to use the canonical
implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 140f54b2-e5fd-459d-ae9e-07bd0fe316b6
📒 Files selected for processing (3)
cmd/gh-oss-stats/badgeConfig.gocmd/gh-oss-stats/badgeGeneration.gocmd/gh-oss-stats/badgeGeneration_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/gh-oss-stats/badgeConfig.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes / Validation
Documentation