Skip to content

feat: add markdown styles via glamour#8

Merged
juanbzz merged 9 commits into
mainfrom
feat/markdown
Jul 22, 2025
Merged

feat: add markdown styles via glamour#8
juanbzz merged 9 commits into
mainfrom
feat/markdown

Conversation

@juanbzz

@juanbzz juanbzz commented Jul 22, 2025

Copy link
Copy Markdown
Owner

No description provided.

@github-actions

Copy link
Copy Markdown

🤖 miso Code Review

📝 Review for cmd/main/main.go

Change Impact Analysis

[🟢 Safe] The changes primarily add new functionality (rich terminal output) without modifying existing behavior.

Code Quality Issues

🟡 Warning: Error Handling in Rich Output

  • Lines 252-272: Nested error handling could be simplified
  • Multiple fallback paths with duplicate fmt.Println(formattedContent)
  • Suggestion: Extract rich rendering into a separate function with a single error return
func renderRichOutput(content string) (string, error) {
    renderer, err := glamour.NewTermRenderer(
        glamour.WithAutoStyle(),
        glamour.WithWordWrap(80),
    )
    if err != nil {
        return "", fmt.Errorf("failed to create renderer: %w", err)
    }
    
    rendered, err := renderer.Render(content)
    if err != nil {
        return "", fmt.Errorf("failed to render content: %w", err)
    }
    
    return rendered, nil
}

💡 Suggestion: Command Line Flag Documentation

  • Line 43: The Style field documentation could be more descriptive
  • Add details about what "rich" output means and when to use it
  • Example: help:"Output style: plain (default) or rich (formatted with colors and markdown)"

Consistency & Patterns

✅ Good: Dependency Management

  • Appropriate use of go.mod/go.sum for new glamour dependency
  • All required indirect dependencies properly tracked

✅ Good: Error Handling Pattern

  • Follows project's pattern of graceful fallback on errors
  • Maintains existing functionality when rich rendering fails

⚠️ Minor Issue: Command Line Interface

  • Line 43: Style flag uses short form -s which could conflict with future flags
  • Consider using longer form --style for better clarity and maintainability

The changes follow Go best practices by:

  • Maintaining small, focused functions
  • Proper error handling with fallbacks
  • Clear dependency management
  • Consistent command-line flag patterns
📝 Review for internal/prompts/code-review.go

I'll analyze the changes focusing on the specific diff and the provided architecture guide.

Change Impact Analysis

🟢 Safe - Changes primarily add new functionality without breaking existing code

Code Quality Issues

🟡 Warning: Error Handling in Rich Output (Line 252-272)

  • The error handling for glamour rendering falls back silently to plain output
  • Should log errors to help with debugging issues
  • Fix: Add error logging before fallback
if err != nil {
    log.Printf("Failed to initialize glamour renderer: %v", err)
    fmt.Println(formattedContent)
    return
}

💡 Suggestion: Style Enum Validation (Line 40)

  • The Style field uses enum but validation is implicit through Kong
  • Consider adding explicit validation or constants
  • Fix: Define constants for styles
const (
    StylePlain = "plain"
    StyleRich  = "rich"
)

Consistency & Patterns

✅ Good: Dependency Management

  • New glamour dependency properly added to go.mod/go.sum
  • Dependencies organized logically in import groups

✅ Good: Feature Organization

  • Rich output formatting code kept within review command logic
  • Follows project structure guidelines

⚠️ Minor Issue: Configuration Pattern

  • New style option added directly to command struct
  • Consider moving display configuration to separate config struct for better organization
type DisplayConfig struct {
    Style string
    Message string
}

type ReviewCmd struct {
    File string
    Verbose bool
    Display DisplayConfig
}

Additional Notes

  • The changes follow Go best practices for error handling with appropriate fallbacks
  • New functionality is added in a modular way without breaking existing code
  • Dependencies are properly managed with minimal version constraints
  • Code organization maintains existing patterns

The changes are generally well-structured and follow the project's architecture guidelines. Main improvements needed are around error logging and configuration organization.

@github-actions

github-actions Bot commented Jul 22, 2025

Copy link
Copy Markdown

🍲 miso Code review

📝 Review for cmd/main/main.go

I'll analyze the changes focusing on the specific diff and the provided guidelines.

Change Impact Analysis

🟢 Safe - Changes are primarily additive and improve functionality without breaking existing behavior

Code Quality Issues

🟡 Warning: Multiple Command Line Flag Changes

  • Lines 37-43: The ReviewCmd struct changes modify existing CLI flags
  • Impact: While not breaking, renaming --style to --output-style should be documented in release notes
  • Fix: Add migration notes in CHANGELOG.md (already done correctly)

💡 Suggestion: Error Handling Improvement

  • Lines 253-265: Consider extracting rich output rendering to a separate function
  • Why: Follows "Keep Functions Small" principle from architecture guide
  • Fix: Move rendering logic to something like:
func (r *ReviewCmd) renderOutput(content string) error {
    if r.OutputStyle == "rich" {
        return renderRichOutput(content)
    }
    fmt.Println(content)
    return nil
}

Consistency & Patterns

✅ Good: Error Wrapping

  • Lines 667-680: New renderRichOutput function properly wraps errors with context
  • Follows error handling best practices from architecture guide

✅ Good: Dependency Management

  • Lines 5-11: New imports are properly organized and grouped
  • Follows project structure guidelines

✅ Good: Feature Documentation

  • Lines 7-13: CHANGELOG.md updates properly document all changes
  • Includes both feature additions and flag renaming

⚠️ Minor Issue: Emoji Consistency

  • Lines 55-110: Changed emoji from 🤖 to 🍲 across multiple files
  • While not critical, consider documenting emoji standards in style guide
  • Fix: Add emoji usage guidelines to documentation

The changes overall are well-structured and follow the project's architecture guidelines. The main suggestions are around documentation and function organization, but no critical issues were found.

📝 Review for internal/prompts/code-review.go

I'll analyze the changes focusing on the specific diff and the provided guidelines.

Change Impact Analysis

🟢 Safe - Changes are primarily cosmetic and additive, with no breaking API changes.

Code Quality Issues

🟡 Warning:

  • Lines +253-268: Error handling in renderRichOutput could be improved
    • Current: Error is logged but execution continues with unformatted output
    • Why: This creates inconsistent output formatting for users
    • Fix: Consider returning the error to caller:
    if r.OutputStyle == "rich" {
        rendered, err := renderRichOutput(formattedContent)
        if err != nil {
            return fmt.Errorf("failed to render rich output: %w", err)
        }
        fmt.Print(rendered)
    }

💡 Suggestion:

  • Lines +664-682: renderRichOutput function could be moved to a dedicated formatting package
    • Why: Following "Directory per Feature" guideline from architecture guide
    • Fix: Create internal/format package and move rendering logic there

Consistency & Patterns

✅ Good:

  • Lines +37,+38: New flag follows existing CLI flag pattern using kong tags
  • Lines +664-682: Error wrapping follows project guidelines using fmt.Errorf with %w
  • Lines +5,+6,+7: Import grouping maintains standard Go style

⚠️ Minor Issue:

  • Lines +55,+82,+98,+110: Emoji change from 🤖 to 🍲
    • While not critical, ensure this change is intentional and documented
    • Consider adding a comment explaining the branding change

Additional Notes:

  1. The addition of rich output formatting is a good enhancement that maintains backward compatibility through the default "plain" mode
  2. New dependencies (glamour) are appropriate for the formatting functionality
  3. Error handling follows Go best practices with proper wrapping and context

Overall, the changes are well-structured and follow the project's architectural guidelines. The main improvement areas are around error handling consistency and package organization.

@juanbzz
juanbzz merged commit fc1d826 into main Jul 22, 2025
1 check passed
@juanbzz
juanbzz deleted the feat/markdown branch July 22, 2025 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant