Improving DX, adding a --ci option and listing generated reports in output #68
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #68 +/- ##
==========================================
+ Coverage 53.49% 53.69% +0.20%
==========================================
Files 42 43 +1
Lines 4827 4894 +67
==========================================
+ Hits 2582 2628 +46
- Misses 2016 2038 +22
+ Partials 229 228 -1 ☔ View full report in Codecov by Sentry. |
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces significant enhancements to the command-line interface (CLI) of the application, including a new Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (25)
src/Report/GeneratedReport.go (3)
3-15: Consider using constants for the Type field and adding validation.While the struct is well-organized, consider these improvements:
- Define constants for possible report types to prevent typos and ensure consistency
- Add validation methods to ensure required fields are not empty
- Consider using custom types for fields that need validation
Here's a suggested implementation:
+// ReportType represents the type of generated report +type ReportType string + +const ( + HTMLReport ReportType = "html" + JSONReport ReportType = "json" + MarkdownReport ReportType = "markdown" ) type GeneratedReport struct { // The path to the generated report Path string // The type of the report - Type string + Type ReportType // Description of the report Description string // Icon of the report Icon string } +// Validate ensures all required fields are properly set +func (r *GeneratedReport) Validate() error { + if r.Path == "" { + return fmt.Errorf("path is required") + } + return nil +}
4-14: Enhance field documentation with examples and more details.While the current comments are good, they could be more helpful with examples and additional context.
Here's a suggested improvement for the comments:
type GeneratedReport struct { - // The path to the generated report + // Path is the filesystem location where the report is saved + // Example: "/tmp/reports/analysis-2024-01-20.html" Path string - // The type of the report + // Type indicates the report format (e.g., "html", "json", "markdown") Type string - // Description of the report + // Description provides a human-readable summary of the report's contents + // Example: "Code quality analysis report for main branch" Description string - // Icon of the report + // Icon represents a visual identifier for the report type + // Example: "📊" for charts, "📝" for documents Icon string }
3-15: Consider implementing common Go interfaces.To improve usability and integration with Go's standard library, consider implementing common interfaces like
fmt.Stringer.Here's a suggested addition:
// String implements fmt.Stringer interface func (r GeneratedReport) String() string { return fmt.Sprintf("%s report at %s: %s", r.Type, r.Path, r.Description) }src/Report/Reporter.go (1)
8-10: Add GoDoc comments for better documentation.Consider adding documentation comments to describe the interface and its method.
+// Reporter defines the contract for generating reports from analyzed files type Reporter interface { + // Generate produces reports from the analyzed files and project aggregation + // Returns a slice of GeneratedReport and any error encountered during generation Generate(files []*pb.File, projectAggregated Analyzer.ProjectAggregated) ([]GeneratedReport, error) }src/Report/Json/JsonReportGenerator_test.go (1)
Line range hint
1-108: Consider improving test structure and coverage.While the test is thorough in validating the JSON structure, there are some potential improvements:
- Consider using test fixtures instead of hardcoded values
- The error checks are repetitive and could be consolidated
- Missing test cases for error scenarios (e.g., invalid file paths, permission issues)
Here's a suggested refactor to improve the test structure:
func TestGenerateJson(t *testing.T) { + // Setup test cases + tests := []struct { + name string + files []*pb.File + aggregated Analyzer.ProjectAggregated + shouldError bool + }{ + { + name: "valid case", + files: []*pb.File{{Path: "file1.php"}}, + aggregated: Analyzer.ProjectAggregated{ + Combined: Analyzer.Aggregated{ + ConcernedFiles: []*pb.File{{Path: "file1.php"}}, + }, + }, + shouldError: false, + }, + // Add more test cases here + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reportPath := "/tmp/report.json" + generator := &JsonReportGenerator{ReportPath: reportPath} + defer os.Remove(reportPath) + + _, err := generator.Generate(tt.files, tt.aggregated) + if tt.shouldError { + assert.Error(t, err) + return + } + + assert.NoError(t, err) + assertValidJsonReport(t, reportPath) + }) + } } + +func assertValidJsonReport(t *testing.T, path string) { + t.Helper() + + assert.FileExists(t, path) + + bytes, err := ioutil.ReadFile(path) + assert.NoError(t, err) + + var report map[string]interface{} + err = json.Unmarshal(bytes, &report) + assert.NoError(t, err) + + concernedFiles, ok := report["concernedFiles"].([]interface{}) + assert.True(t, ok) + assert.Len(t, concernedFiles, 1) + + concernedFile, ok := concernedFiles[0].(map[string]interface{}) + assert.True(t, ok) + assert.Contains(t, concernedFile, "path") +}src/Configuration/ConfigurationLoader.go (3)
24-47: Consider enhancing error messages for better debugging.While the error handling is present, the errors could be more descriptive to aid in debugging, especially in CI environments.
Consider wrapping errors with context:
if err != nil { - return cfg, err + return cfg, fmt.Errorf("failed to open configuration file %s: %w", filename, err) }if err != nil { - return cfg, err + return cfg, fmt.Errorf("failed to decode configuration file %s: %w", filename, err) }
51-58: Enhance error message for YAML parsing failures.Similar to the previous suggestion, the error message could be more descriptive.
Consider wrapping the error:
if err != nil { - return cfg, err + return cfg, fmt.Errorf("failed to parse YAML configuration: %w", err) }
62-71: Improve validation error message.The error message for empty FilenameToChecks could be more informative.
Consider a more descriptive error message:
if len(c.FilenameToChecks) == 0 { - return errors.New("No filename to check") + return errors.New("configuration loader requires at least one filename to check") }src/Configuration/Configuration.go (3)
30-31: Add documentation for the new field.The
IsComingFromConfigFilefield would benefit from a documentation comment explaining its purpose and usage, especially since it's an exported field.Storage *Storage.Workdir + // IsComingFromConfigFile indicates whether the configuration was loaded from a config file + // rather than being created programmatically or via command line arguments IsComingFromConfigFile bool
40-43: Fix the malformed comment and add proper Go documentation.The current comment is malformed. It should follow Go's documentation conventions.
-// function HasReports() bool { +// HasReports returns true if any report type (HTML, Markdown, or JSON) is configured func (c *ConfigurationReport) HasReports() bool {
74-74: Fix indentation to match other fields.The indentation of the new field initialization doesn't align with other fields in the struct initialization.
- IsComingFromConfigFile: false, + IsComingFromConfigFile: false,src/Report/Markdown/MarkdownReportGenerator.go (1)
28-32: LGTM: Good use of interface return type.Returning the interface type instead of the concrete implementation improves modularity and follows the interface segregation principle. This change makes the code more flexible and easier to test.
src/Cli/ScreenHome.go (3)
78-79: Consider moving the style definition to package-level vars.While the styling enhances the output, the style definition could be moved to the package-level vars section for better reusability and consistency across the package.
// General stuff for styling the view var ( term = termenv.EnvColorProfile() + nonInteractiveStyle = lipgloss.NewStyle(). + Foreground(lipgloss.Color("#666666")). + Italic(true) ) // ... later in the Render method ... - var style = lipgloss.NewStyle().Foreground(lipgloss.Color("#666666")).Italic(true) - fmt.Println(style.Render("No interactive mode detected.")) + fmt.Println(nonInteractiveStyle.Render("No interactive mode detected."))
Line range hint
208-208: Improve the comment's clarity.The comment "issue when navigating back to the main screen" is vague. Consider documenting the specific issue this fixes and why
fillInScreensneeds to be called here.- // issue when navigating back to the main screen + // Ensure screens are properly repopulated when navigating back to the main screen + // This fixes potential screen list inconsistencies after model updates
Line range hint
19-31: Consider adding documentation about screen lifecycle.The
ScreenHomestruct manages multiple screens and their lifecycle (creation, navigation, updates). Consider adding documentation that explains:
- How screens are managed and their lifecycle
- When screens are created/destroyed
- How navigation between screens works
- How screen state is maintained
This would improve maintainability and help future contributors understand the screen management system better.
main.go (3)
87-88: Consider enhancing the JSON report flag documentationWhile the implementation is correct, consider adding more details to the usage description, such as the expected format or purpose of the JSON report.
- Usage: "Generate a report in JSON format", + Usage: "Generate a report in JSON format for programmatic analysis",
97-102: Enhance CI mode flag documentationThe --ci flag's usage description should explicitly state what it enables (non-interactive mode and default report generation).
- Usage: "Enable CI mode", + Usage: "Enable CI mode (enables non-interactive mode and generates default HTML/Markdown reports)",
132-134: Consider enhancing the welcome messageWhile the styling is good, consider adding version information to the welcome message for better user experience.
- fmt.Println(style.Render("\n🦫 AST Metrics is a language-agnostic static code analyzer.")) + fmt.Println(style.Render(fmt.Sprintf("\n🦫 AST Metrics v%s - A language-agnostic static code analyzer", version)))src/Report/Json/JsonReportGenerator.go (1)
37-37: Refine error messages for clarityThe error messages could be more informative and consistent. Consider using error wrapping and providing clearer context to aid in debugging.
Apply the following diff to improve the error messages:
- return nil, fmt.Errorf("can not clean report err: %s", err.Error()) + return nil, fmt.Errorf("failed to clean report: %w", err) - return nil, fmt.Errorf("can not serialize report to JSON err: %s", err.Error()) + return nil, fmt.Errorf("failed to serialize report to JSON: %w", err) - return nil, fmt.Errorf("can not save report to path %s err: %s", j.ReportPath, err.Error()) + return nil, fmt.Errorf("failed to save report to path %s: %w", j.ReportPath, err)Also applies to: 43-43, 49-49
src/Command/AnalyzeCommand.go (4)
6-6: Consider using 'pterm' for consistent output stylingThe
fmtpackage is imported at line 6~ and used for printing output messages later in the code. To maintain consistent styling and take advantage ofpterm's capabilities, consider usingptermmethods for all user-facing messages instead offmt.Println. This will ensure uniformity in the application's output and enhance the user experience.
179-200: Handle report generation errors without halting subsequent processesIn the report generation loop (lines 193~ to 200~), if an error occurs while generating a report, the function logs the error and returns immediately (line 197~). This stops the generation of any remaining reports, which might not be desirable. To ensure that all possible reports are generated, consider logging the error and continuing with the next reporter.
Apply this diff to continue generating reports even if one fails:
if err != nil { pterm.Error.Println("Cannot generate report: " + err.Error()) - return err + continue }
254-266: Use 'pterm' for consistent and styled output messagesLines 257~ to 265~ use
fmt.Printlnto display messages to the user. For consistency and to enhance output styling, consider replacingfmt.Printlnwithptermmethods such aspterm.Println,pterm.Info.Println, orpterm.Success.Println. This will provide a more uniform and visually appealing interface.Apply this diff to update the output methods:
- fmt.Println("") - fmt.Println("📁 These reports have been generated:") + pterm.Println("") + pterm.Println("📁 These reports have been generated:") for _, report := range generatedReports { - fmt.Println("\n ✔ " + report.Path + " (" + report.Type + ")") - fmt.Println("\n " + report.Description) + pterm.Success.Println("\n ✔ " + report.Path + " (" + report.Type + ")") + pterm.Info.Println("\n " + report.Description) } - fmt.Println("") + pterm.Println("")
276-284: Enhance user messages with 'pterm' for better stylingLines 278~ and 282~ use
fmt.Printlnto display informational messages to the user. Usingptermmethods can improve the presentation and maintain consistency with other parts of the application whereptermis used.Apply this diff to replace
fmt.Printlnwithptermmethods:- fmt.Println("\n💡 We noticed that you haven't yet created a configuration file. You can create a .ast-metrics.yaml configuration file by running: ast-metrics init") - fmt.Println("") + pterm.Info.Println("\n💡 We noticed that you haven't yet created a configuration file. You can create a .ast-metrics.yaml configuration file by running: ast-metrics init") + pterm.Println("") - fmt.Println("\n🌟 If you like AST Metrics, please consider starring the project on GitHub: https://github.com/Halleck45/ast-metrics/. Thanks!") - fmt.Println("") + pterm.Info.Println("\n🌟 If you like AST Metrics, please consider starring the project on GitHub: https://github.com/Halleck45/ast-metrics/. Thanks!") + pterm.Println("")src/Report/Html/HtmlReportGenerator.go (2)
Line range hint
37-131: Handle errors returned byGenerateLanguagePagecallsIn the
Generatemethod, the errors returned byv.GenerateLanguagePageare not being checked. Ignoring these errors could lead to silent failures and incomplete report generation.Consider capturing and handling the errors appropriately:
// Example for one call if err := v.GenerateLanguagePage("index.html", "All", projectAggregated.Combined, files, projectAggregated); err != nil { return nil, err } // Repeat for other calls to GenerateLanguagePage for language, currentView := range projectAggregated.ByProgrammingLanguage { if err := v.GenerateLanguagePage("index.html", language, currentView, files, projectAggregated); err != nil { return nil, err } }
41-41: Return empty slice instead ofnilwhen no report is generatedWhen
v.ReportPathis empty, theGeneratemethod returnsnil, nil. In Go, it's idiomatic to return an empty slice rather thannilto avoid potentialnilpointer dereferences in the calling code.Update the return statement:
- return nil, nil + return []Report.GeneratedReport{}, nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
main.go(5 hunks)src/Cli/ScreenHome.go(2 hunks)src/Cli/ScreenHtmlReport.go(1 hunks)src/Command/AnalyzeCommand.go(5 hunks)src/Configuration/Configuration.go(3 hunks)src/Configuration/ConfigurationLoader.go(2 hunks)src/Report/GeneratedReport.go(1 hunks)src/Report/Html/HtmlReportGenerator.go(4 hunks)src/Report/Json/JsonReportGenerator.go(1 hunks)src/Report/Json/JsonReportGenerator_test.go(1 hunks)src/Report/Markdown/MardownReportGenerator_test.go(2 hunks)src/Report/Markdown/MarkdownReportGenerator.go(4 hunks)src/Report/Reporter.go(1 hunks)
🔇 Additional comments (20)
src/Report/GeneratedReport.go (1)
1-1: LGTM! Package name follows Go conventions.
src/Report/Reporter.go (1)
8-10: LGTM! Well-designed interface following SOLID principles.
The Reporter interface is well-designed with:
- Single responsibility (SRP)
- Clear method signature
- Proper error handling
- Clean parameter types
src/Report/Json/JsonReportGenerator_test.go (1)
30-30: Verify the Generate method's API change across the codebase.
The change to ignore the first return value suggests an API modification in the Generate method. Let's verify this change is consistent across the codebase.
✅ Verification successful
Let me gather more information about the Generate method's implementation to verify the API change.
Based on the gathered information, I can now provide the final response:
The API change to ignore the first return value is consistent across the codebase
The verification shows that:
- The
Reporterinterface insrc/Report/Reporter.godefinesGenerateto return([]GeneratedReport, error) - All implementations consistently ignore the first return value in test files (
JsonReportGenerator_test.go,MardownReportGenerator_test.go) - The actual usage in production code (
AnalyzeCommand.go,ScreenHtmlReport.go) shows proper handling of both return values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Generate method signature and usage across different report generators
# Check the Generate method signature in report generators
echo "Checking Generate method signatures:"
ast-grep --pattern 'func $_($$) ($_, error)' | rg 'Generate'
# Check usage patterns of Generate method
echo "\nChecking Generate method usage patterns:"
rg -A 1 '\.Generate\('
Length of output: 1032
Script:
#!/bin/bash
# Check the actual implementation of Generate methods
echo "Checking Generate method implementations:"
ast-grep --pattern 'func ($$) Generate($_, $_) ($_, error) {
$$$
}'
# Check interface definitions that might define Generate
echo "\nChecking interface definitions:"
ast-grep --pattern 'type $_ interface {
$$$
Generate($_, $_) ($_, error)
$$$
}'
Length of output: 534
src/Configuration/ConfigurationLoader.go (3)
4-20: LGTM! Well-structured initialization code.
The import organization, struct definition, and constructor follow Go best practices. The default configuration filenames follow common patterns for configuration files.
Line range hint 73-123: LGTM! Well-documented default configuration.
The default configuration template is comprehensive and includes helpful comments that improve the developer experience, aligning well with the PR's DX improvement objectives.
24-47: Verify configuration behavior in CI environments.
Since this PR adds CI-specific features, we should ensure the configuration loading behavior is appropriate for CI environments.
✅ Verification successful
Based on the analysis of the shell script results, I can now provide a final response:
Configuration handling is appropriate for CI environments
The codebase properly handles CI-specific configuration through:
- A dedicated
--ciflag that enables CI mode (non-interactive, with HTML and Markdown reports) - The
IsComingFromConfigFileflag is used appropriately to detect and handle configuration file presence - Configuration loading behavior is consistent across both CI and non-CI environments
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any CI-specific configuration handling in the codebase
rg -l "IsComingFromConfigFile|--ci" --type go
# Look for any CI-related test cases
rg -l "TestCI|ci.*test" --type go
Length of output: 189
Script:
#!/bin/bash
# Check how IsComingFromConfigFile is used in the codebase
rg "IsComingFromConfigFile" -B 2 -A 2
# Check for CI-related command line flags or environment handling
rg "ci|CI" -B 2 -A 2 --type go
# Look at the configuration struct definition
ast-grep --pattern 'type Configuration struct {
$$$
}'
Length of output: 59966
src/Configuration/Configuration.go (1)
Line range hint 30-74: Changes look good and align well with PR objectives!
The additions improve the developer experience by:
- Providing clarity about configuration sources
- Adding a convenient way to check for configured reports
- Supporting the new --ci option through better configuration tracking
These changes lay good groundwork for the report listing feature mentioned in the PR objectives.
src/Report/Markdown/MarkdownReportGenerator.go (4)
15-15: LGTM: Import addition is necessary for the interface return type.
34-38: LGTM: Consistent error handling pattern.
The error handling follows a consistent pattern of returning (nil, err) throughout the method, which makes the code more predictable and maintainable.
Also applies to: 44-45, 50-51, 56-57, 71-72, 77-78, 84-85, 93-94
97-104: LGTM: Well-structured report metadata.
The report metadata structure provides clear and useful information about the generated report, which aligns well with the PR's objective of improving developer experience.
Line range hint 34-105: Verify CI mode handling.
The PR objectives mention adding a --ci option, but there's no visible handling of CI-specific behavior in the Generate method. Let's verify if this is handled elsewhere or needs to be added here.
✅ Verification successful
Let me gather more context about the CI mode implementation.
Let me check how the CI flag is used to affect the application behavior.
Let me try one final search to understand how the CI mode is handled in the application configuration.
CI mode is correctly handled - no changes needed in MarkdownReportGenerator
The CI mode is properly implemented at the application level where it acts as an alias for multiple flags including --non-interactive and --report-markdown. The MarkdownReportGenerator doesn't need specific CI mode handling because:
- The CI mode automatically enables markdown report generation through the flag aliasing
- The non-interactive behavior is handled at the application level (main.go)
- The generator already works correctly when the report is requested, regardless of the mode
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CI mode handling in the codebase
echo "Searching for CI mode flag handling..."
rg --type go -i "ci.*flag|flag.*ci"
echo "Searching for CI mode specific behavior..."
rg --type go -i "ci.*mode|mode.*ci"
Length of output: 413
Script:
#!/bin/bash
# Check the CI mode implementation in main.go
echo "Checking CI mode implementation in main.go..."
rg -A 10 -B 2 'Enable CI mode' main.go
# Check if there are any other CI-related configurations
echo -e "\nChecking for other CI-related configurations..."
ast-grep --pattern 'var $_ = cli.BoolFlag {
$$$
Usage: "Enable CI mode",
$$$
}'
Length of output: 720
Script:
#!/bin/bash
# Check how CI flag affects the application
echo "Checking CI flag usage..."
rg -A 5 -B 5 'ctx\.Bool\("ci"\)' main.go
# Check for any CI-related configuration in the app initialization
echo -e "\nChecking app initialization for CI mode..."
ast-grep --pattern 'func $_(ctx *cli.Context) error {
$$$
if ctx.Bool("ci") {
$$$
}
$$$
}'
Length of output: 309
Script:
#!/bin/bash
# Search for configuration handling related to CI mode
echo "Searching for configuration handling..."
rg -A 10 "func.*initConfig" main.go
# Check for any non-interactive mode settings which might be related to CI
echo -e "\nChecking non-interactive mode settings..."
rg -A 5 "non-interactive" main.go
Length of output: 837
src/Cli/ScreenHome.go (1)
8-8: LGTM: Good choice for terminal styling!
The addition of lipgloss is a good choice for enhancing the CLI output styling, which aligns well with the DX improvement objectives.
main.go (2)
5-5: LGTM: Import additions are properly utilized
The new imports for fmt and lipgloss are appropriately used for enhanced output formatting.
Also applies to: 8-8
125-125: LGTM: Proper handling of interactive mode flags
The condition correctly handles both --non-interactive and --ci flags.
src/Report/Json/JsonReportGenerator.go (4)
20-23: Change factory function to return interface for better abstraction
Modifying NewJsonReportGenerator to return the Report.Reporter interface instead of a concrete *JsonReportGenerator enhances flexibility. This change promotes abstraction and allows for swapping different implementations without altering the calling code.
27-27: Update 'Generate' method signature to return reports
Changing the Generate method to return ([]Report.GeneratedReport, error) aligns it with the updated reporting framework. This allows the method to return detailed report metadata alongside any errors, improving the integration with the rest of the reporting system.
30-30: Handle empty 'ReportPath' explicitly
When ReportPath is empty, the method returns (nil, nil). Ensure that the calling code correctly handles this scenario to prevent unexpected behavior due to missing reports.
Please confirm that the callers of this method check for a nil report slice when ReportPath is empty.
52-60: Construct and return report metadata
Building the reports slice with detailed metadata and returning it ensures that the generated report information is available to the caller. This practice enhances the reporting capabilities by providing structured data about the generated reports.
src/Report/Html/HtmlReportGenerator.go (2)
31-34: Update constructor to return the Report.Reporter interface
The NewHtmlReportGenerator function now returns the Report.Reporter interface instead of a concrete *HtmlReportGenerator type. This promotes abstraction and allows for more flexible implementations of report generators.
122-131: Properly construct and return the generated report metadata
The addition of the reports slice provides structured metadata about the generated reports, which is beneficial for consumers of the Generate method.
| @@ -71,7 +71,7 @@ func TestGenerateWithTemplateFiles(t *testing.T) { | |||
| // Create a temporary template file | |||
| ioutil.WriteFile("/tmp/templates/index.md", []byte("Test template"), 0644) | |||
|
|
|||
| err := generator.Generate(files, projectAggregated) | |||
| _, err := generator.Generate(files, projectAggregated) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for template-based report generation results.
Similar to the previous test, this test should verify the content and structure of the returned GeneratedReport, especially in the context of template-based generation.
func TestGenerateWithTemplateFiles(t *testing.T) {
generator := &MarkdownReportGenerator{ReportPath: "/tmp/report.md"}
files := []*pb.File{}
projectAggregated := Analyzer.ProjectAggregated{}
// Create a temporary template file
ioutil.WriteFile("/tmp/templates/index.md", []byte("Test template"), 0644)
- _, err := generator.Generate(files, projectAggregated)
+ result, err := generator.Generate(files, projectAggregated)
+
+ // Verify the returned report metadata
+ expectedPath := "/tmp/report.md"
+ if result.Path != expectedPath {
+ t.Errorf("Expected report path %v, got %v", expectedPath, result.Path)
+ }
+ // Add other assertions for template-specific metadata📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _, err := generator.Generate(files, projectAggregated) | |
| result, err := generator.Generate(files, projectAggregated) | |
| // Verify the returned report metadata | |
| expectedPath := "/tmp/report.md" | |
| if result.Path != expectedPath { | |
| t.Errorf("Expected report path %v, got %v", expectedPath, result.Path) | |
| } | |
| // Add other assertions for template-specific metadata |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
src/Cli/ScreenEnd.go (3)
13-38: Add documentation for the 'tea' field.While most fields are documented, the
teafield lacks proper documentation. Consider adding a descriptive comment to explain its purpose.// program - tea *tea.Program + // tea is the Bubble Tea program instance used for terminal UI rendering + tea *tea.Program
84-85: Add implementation or documentation for Reset method.The
Resetmethod is empty without any explanation. Either implement the required functionality or add a comment explaining why it's intentionally empty.func (r *ScreenEnd) Reset(files []*pb.File, projectAggregated Analyzer.ProjectAggregated) { + // TODO: Implement reset functionality or document why this method is intentionally empty }
58-81: Refactor Render method for better maintainability.Several improvements could enhance the maintainability of the Render method:
- Extract string literals as constants
- Simplify multiple consecutive newlines
- Add nil check for reports slice
+const ( + reportsHeader = "📁 These reports have been generated:" + configTip = "💡 We noticed that you haven't yet created a configuration file. You can create a .ast-metrics.yaml configuration file by running: ast-metrics init" + starMessage = "🌟 If you like AST Metrics, please consider starring the project on GitHub: https://github.com/Halleck45/ast-metrics/. Thanks!" +) func (r *ScreenEnd) Render() { + if r.reports == nil { + return + } + // List reports if r.configuration.Reports.HasReports() { - fmt.Println("\n") - fmt.Println("📁 These reports have been generated:") + fmt.Println("\n" + reportsHeader) for _, report := range r.reports { fmt.Println("\n ✔ " + report.Path + " (" + report.Type + ")") fmt.Println("\n " + report.Description) } - - fmt.Println("") } // Tips if configuration file does not exist if !r.configuration.IsComingFromConfigFile { - fmt.Println("\n💡 We noticed that you haven't yet created a configuration file. You can create a .ast-metrics.yaml configuration file by running: ast-metrics init") - fmt.Println("") + fmt.Println("\n" + configTip) } - fmt.Println("\n🌟 If you like AST Metrics, please consider starring the project on GitHub: https://github.com/Halleck45/ast-metrics/. Thanks!") - fmt.Println("") + fmt.Println("\n" + starMessage + "\n") }src/Report/Json/JsonReportGenerator.go (2)
20-24: LGTM: Good use of interface-based design.Returning the Report.Reporter interface instead of the concrete type improves abstraction and flexibility. This change follows the Go idiom of "accept interfaces, return structs" and allows for better testing and maintainability.
52-60: LGTM: Comprehensive report metadata structure.The GeneratedReport structure provides useful metadata about the generated JSON file. Consider adding file size or generation timestamp to the metadata for better tracking and debugging capabilities.
reports := []Report.GeneratedReport{ { Path: j.ReportPath, Type: "file", Description: "The JSON report allows scripts to parse the results programmatically.", Icon: "📄", + Size: len(jsonReport), + GeneratedAt: time.Now().UTC(), }, }src/Cli/ScreenHome.go (2)
78-79: Consider extracting the style definition.While the styling improves visual feedback, consider moving the style definition to a constants section at the top of the file alongside other style-related variables. This would maintain consistency with the existing pattern (see the
termvariable) and improve reusability.// General stuff for styling the view var ( term = termenv.EnvColorProfile() + nonInteractiveStyle = lipgloss.NewStyle(). + Foreground(lipgloss.Color("#666666")). + Italic(true) ) // ... rest of the file ... // In the Render method: - var style = lipgloss.NewStyle().Foreground(lipgloss.Color("#666666")).Italic(true) - fmt.Println(style.Render("No interactive mode detected.")) + fmt.Println(nonInteractiveStyle.Render("No interactive mode detected."))
Line range hint
1-246: Add tests to improve coverage.According to the PR objectives, this file has 0% test coverage. Consider adding tests for:
- Screen navigation logic
- Model updates
- Non-interactive mode rendering
- Screen initialization
This will help prevent regressions and ensure the UI behaves correctly.
Would you like me to help create a test suite for this file? I can provide examples of test cases that cover the critical paths.
src/Command/AnalyzeCommand.go (1)
178-199: Excellent refactoring of report generation logic.The new implementation using a slice of reporters improves modularity, maintainability, and extensibility.
Consider these improvements:
- Error handling could be more granular to allow partial success:
for _, reporter := range reporters { reports, err := reporter.Generate(allResults, projectAggregated) if err != nil { - pterm.Error.Println("Cannot generate report: " + err.Error()) - return err + log.Errorf("Failed to generate report using %T: %v", reporter, err) + continue } generatedReports = append(generatedReports, reports...) }
- Consider extracting reporter initialization to improve readability:
func (v *AnalyzeCommand) initializeReporters() []Report.Reporter { var reporters []Report.Reporter if v.configuration.Reports.Html != "" { reporters = append(reporters, Html.NewHtmlReportGenerator(v.configuration.Reports.Html)) } // ... similar for other reporters return reporters }Add unit tests for the new report generation logic.
The Codecov report indicates missing coverage. Please add tests for:
- Reporter initialization with different configuration combinations
- Error handling scenarios
- Integration of multiple reporters
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
main.go(5 hunks)src/Cli/ScreenEnd.go(1 hunks)src/Cli/ScreenHome.go(4 hunks)src/Command/AnalyzeCommand.go(3 hunks)src/Report/Json/JsonReportGenerator.go(1 hunks)src/Report/Json/JsonReportGenerator_test.go(1 hunks)src/Report/Markdown/MardownReportGenerator_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- main.go
- src/Report/Json/JsonReportGenerator_test.go
- src/Report/Markdown/MardownReportGenerator_test.go
🔇 Additional comments (9)
src/Cli/ScreenEnd.go (1)
1-11: LGTM!
The package structure and imports are well-organized and appropriate for the functionality.
src/Report/Json/JsonReportGenerator.go (3)
12-12: LGTM: Import addition is necessary for the new return types.
The new import is required to support the updated method signatures returning Report.Reporter and Report.GeneratedReport types.
27-30: LGTM: Improved error handling with early return.
The updated method signature properly returns both the report slice and error. The early return case for empty ReportPath is now more explicit and consistent with the new return type.
37-37: LGTM: Consistent error handling pattern.
Error handling follows a consistent pattern across all error cases:
- Returns nil for the report slice
- Uses fmt.Errorf with descriptive messages
- Includes the original error in the message
Also applies to: 43-43, 49-49
src/Cli/ScreenHome.go (3)
8-8: LGTM: Import addition aligns with styling improvements.
The addition of the lipgloss package is appropriate for enhancing the CLI's visual feedback.
112-112: Verify the impact of GetModel signature change.
The return type change from modelChoices to tea.Model is a breaking change that could affect callers. Let's verify all usages to ensure they've been updated.
✅ Verification successful
Based on the search results, I can now generate the final response:
GetModel signature change is properly implemented across the codebase
All implementations and usages of GetModel() consistently use tea.Model as the return type. The change has been properly propagated across all screen components:
- ScreenHome
- ScreenHtmlReport
- ScreenEnd
- ScreenTableClass
- ScreenSummary
- ScreenRisks
- ScreenByProgrammingLanguage
The interface definition in Screen.go also matches this signature, confirming a consistent implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for GetModel() calls to verify they handle the new return type
rg -A 2 "GetModel\(\)"
Length of output: 3531
242-245: Verify Screen interface implementation.
The addition of GetScreenName suggests implementing a common Screen interface. Let's verify that all screen types implement this method consistently.
✅ Verification successful
Let me gather more information about screen implementations to verify this thoroughly.
GetScreenName implementation is consistent across all screen types
The verification shows that GetScreenName is properly implemented by all screen structs that implement the Screen interface. Each implementation returns a unique, descriptive screen name:
- ScreenHtmlReport: "Generate HTML report"
- ScreenHome: "Home"
- ScreenEnd: "End"
- ScreenTableClass: "Classes and object oriented statistics"
- ScreenSummary: "Overview"
- ScreenRisks: "Top candidates for refactoring"
- ScreenByProgrammingLanguage: (with emoji)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for GetScreenName implementations across all screen types
ast-grep --pattern 'func ($x $y) GetScreenName() string {
$$$
}'
# Search for the Screen interface definition
rg -A 5 "type Screen interface"
Length of output: 339
Script:
#!/bin/bash
# Find all screen type files
fd -e go . src/Cli | grep -i "screen"
# Search for all struct definitions that might be screens
rg "type Screen\w+ struct"
# Look for GetScreenName method implementations with context
rg -A 2 "func .* GetScreenName\(\) string"
Length of output: 2017
src/Command/AnalyzeCommand.go (2)
15-16: LGTM: Import changes align with the report generation refactoring.
The reorganization of imports reflects the improved modular structure for report generation.
261-264: LGTM: Enhanced end screen with report listing.
The addition of generated reports to the end screen improves developer experience by providing clear feedback about the generated artifacts.
| type modelEnd struct { | ||
| } | ||
|
|
||
| func (m modelEnd) Init() tea.Cmd { | ||
| return nil | ||
| } | ||
|
|
||
| func (m *modelEnd) Reset(files []*pb.File, projectAggregated Analyzer.ProjectAggregated) { | ||
| } | ||
|
|
||
| func (m modelEnd) Update(msg tea.Msg) (tea.Model, tea.Cmd) { | ||
| return m, nil | ||
| } | ||
|
|
||
| func (m modelEnd) View() string { | ||
| return "" | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Now I have enough context to provide the final response:
The modelEnd implementation is necessary but can be improved
The modelEnd struct is a required implementation as it's used by ScreenEnd to satisfy the Screen interface contract, which requires a GetModel() method returning a tea.Model. However, its current empty implementation could be enhanced:
- The empty
View()method should return a meaningful message or confirmation of completion - The
Reset()method should handle cleanup if needed - Consider adding documentation explaining its purpose as the final screen
🔗 Analysis chain
Verify the necessity of the modelEnd implementation.
The modelEnd struct appears to be a placeholder implementation of the tea.Model interface. If it's not actively used, consider removing it to reduce unnecessary complexity. If it's intended for future use, please add TODO comments explaining the planned functionality.
Let me search for more context about where this model is being used.
Let me check if there's any interface or contract that requires this tea.Model implementation.
Let me check one of the other screen implementations to understand the pattern better.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if modelEnd is used anywhere in the codebase
rg "modelEnd" --type go
Length of output: 412
Script:
#!/bin/bash
# Check the entire ScreenEnd.go file to understand the context
cat src/Cli/ScreenEnd.go
# Search for any references to ScreenEnd
rg "ScreenEnd" --type go -A 5
Length of output: 4312
Script:
#!/bin/bash
# Look for any interface definitions that ScreenEnd might be implementing
rg "type.*interface" --type go -A 5
# Check for any other screens to understand the pattern
fd -e go -x grep -l "GetModel()" {}
Length of output: 2133
Script:
#!/bin/bash
# Check implementation of another screen for comparison
cat src/Cli/ScreenHome.go
# Also check the Screen interface definition
cat src/Cli/Screen.go
Length of output: 7224
| func (j *JsonReportGenerator) Generate(files []*pb.File, projectAggregated Analyzer.ProjectAggregated) ([]Report.GeneratedReport, error) { | ||
|
|
||
| if j.ReportPath == "" { | ||
| return nil | ||
| return nil, nil | ||
| } | ||
|
|
||
| report := j.buildReport(projectAggregated) | ||
|
|
||
| err := Cleaner.CleanVal(report) | ||
| if err != nil { | ||
| return fmt.Errorf("can not clean report err: %s", err.Error()) | ||
| return nil, fmt.Errorf("can not clean report err: %s", err.Error()) | ||
| } | ||
|
|
||
| // This code serializes the results to JSON | ||
| jsonReport, err := json.Marshal(report) | ||
| if err != nil { | ||
| return fmt.Errorf("can not serialize report to JSON err: %s", err.Error()) | ||
| return nil, fmt.Errorf("can not serialize report to JSON err: %s", err.Error()) | ||
| } | ||
|
|
||
| // This code writes the JSON report to a file | ||
| err = ioutil.WriteFile(j.ReportPath, jsonReport, os.ModePerm) | ||
| if err != nil { | ||
| return fmt.Errorf("can not save report to path %s err: %s", j.ReportPath, err.Error()) | ||
| return nil, fmt.Errorf("can not save report to path %s err: %s", j.ReportPath, err.Error()) | ||
| } | ||
|
|
||
| return nil | ||
| reports := []Report.GeneratedReport{ | ||
| { | ||
| Path: j.ReportPath, | ||
| Type: "file", | ||
| Description: "The JSON report allows scripts to parse the results programmatically.", | ||
| Icon: "📄", | ||
| }, | ||
| } | ||
| return reports, nil |
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for error scenarios in the Generate method
The current test file only covers the happy path. Missing test cases for:
- Empty report path scenario (
ReportPath: "") - File permission errors (non-writable directory)
- JSON marshaling errors (by injecting data that causes marshaling to fail)
- CleanVal errors
Example test cases to add:
func TestGenerateWithEmptyPath(t *testing.T) {
generator := &JsonReportGenerator{ReportPath: ""}
reports, err := generator.Generate(nil, Analyzer.ProjectAggregated{})
assert.Nil(t, err)
assert.Nil(t, reports)
}
func TestGenerateWithNonWritablePath(t *testing.T) {
generator := &JsonReportGenerator{ReportPath: "/root/forbidden/report.json"}
reports, err := generator.Generate([]*pb.File{}, Analyzer.ProjectAggregated{})
assert.NotNil(t, err)
assert.Contains(t, err.Error(), "can not save report")
}🔗 Analysis chain
Verify: Improve test coverage for the Generate method.
According to Codecov, this file has 66.66% coverage with 5 missing lines. Consider adding test cases for:
- Error cases in JSON marshaling
- File writing failures
- Different report metadata configurations
Let me analyze the test coverage further by checking the implementation of error cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check existing test coverage for the Generate method
# Test: Look for existing test cases
rg -A 5 "func Test.*Generate" "src/Report/Json/JsonReportGenerator_test.go"
# Test: Check for error case testing
rg -A 5 "Error|err :=" "src/Report/Json/JsonReportGenerator_test.go"
Length of output: 2037
Script:
#!/bin/bash
# Check for error handling test cases specifically around report path and marshaling
# Look for test cases with empty report path
rg -B 2 -A 2 'ReportPath: ""' "src/Report/Json/JsonReportGenerator_test.go"
# Look for test cases with invalid permissions or paths
rg -B 2 -A 2 'ReportPath: "/invalid/' "src/Report/Json/JsonReportGenerator_test.go"
# Check if there are any negative test cases
rg "func Test.*Generate.*Error" "src/Report/Json/JsonReportGenerator_test.go"
# Look at the full test file to understand the test coverage
cat "src/Report/Json/JsonReportGenerator_test.go"
Length of output: 2794
Summary by CodeRabbit
Release Notes
New Features
--cifor continuous integration mode, enhancing report generation options.GeneratedReportstruct to manage report metadata.Improvements
Bug Fixes