Emit Friendly Message When Component Docs Not Found #1032
Conversation
📝 WalkthroughWalkthroughThe pull request modifies the error handling and logging mechanisms in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used🪛 golangci-lint (1.62.2)cmd/docs.go12-12: import "github.com/charmbracelet/log" imported without alias but must be with alias "log" according to config (importas) 🪛 GitHub Check: golangcicmd/docs.go[failure] 12-12: [golangci] cmd/docs.go#L12 [failure] 80-80: [golangci] cmd/docs.go#L80 [failure] 72-72: [golangci] cmd/docs.go#L72 [failure] 75-75: [golangci] cmd/docs.go#L75 ⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (4)
✨ Finishing Touches
🪧 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/docs.go (1)
73-79: Consider consolidating README error messages for consistency.Since we're streamlining error messages, we could also simplify this error handling block to match the same pattern:
if _, err := os.Stat(readmePath); err != nil { - if os.IsNotExist(err) { - u.LogErrorAndExit(fmt.Errorf("No README found for component: %s", info.Component)) - } else { - u.LogErrorAndExit(fmt.Errorf("Component %s not found", info.Component)) - } + u.LogErrorAndExit(fmt.Errorf("No README found for component: %s", info.Component)) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/docs.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (1)
cmd/docs.go (1)
68-70: LGTM! Clean consolidation of error handling.The combined error check simplifies the control flow while maintaining clear error messaging. This aligns well with the PR's goal of providing a more consistent user experience.
|
Please add a test case similar to this one: https://github.com/cloudposse/atmos/blob/main/tests/test-cases/core.yaml#L67-L81 that checks for non-zero exit code, and a helpful error message. |
cmd/docs.go
Outdated
| @@ -83,7 +80,7 @@ var docsCmd = &cobra.Command{ | |||
|
|
|||
| readmeContent, err := os.ReadFile(readmePath) | |||
| if err != nil { | |||
| u.LogErrorAndExit(err) | |||
| u.LogErrorAndExit(fmt.Errorf("Loading README file for component: %s", info.Component)) | |||
There was a problem hiding this comment.
do we want to show the actual err here?
There was a problem hiding this comment.
@aknysh is correct, this doesn't achieve the desired outcome. We handle errors today, but it's not friendly.
Here's what we want:
- If the component folder is not found, say "Component not found"
- If the component lacks a README.md, say "Documentation is missing for the component asd. Consider adding a
README.mdto provide more context and details."
There was a problem hiding this comment.
Let’s use l.Debug to emit the actual error, and print a friendly one here.
There was a problem hiding this comment.
That would be useful
There was a problem hiding this comment.
Also, use l.Fatal instead of u.LogErrorAndExit (deprecated after moving to new logger)
osterman
changed the title
|
💥 This pull request now has conflicts. Could you fix it @codercr? 🙏 |
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (5)
cmd/docs.go (5)
10-10: Adjust import alias to follow project conventions.The static analysis tool flags that the import alias "l" for charmbracelet/log should be "log" according to project config.
- l "github.com/charmbracelet/log" + log "github.com/charmbracelet/log"Remember to update all references to "l" throughout the code.
🧰 Tools
🪛 golangci-lint (1.62.2)
10-10: import "github.com/charmbracelet/log" imported as "l" but must be "log" according to config
(importas)
🪛 GitHub Check: golangci
[failure] 10-10: [golangci] cmd/docs.go#L10
import "github.com/charmbracelet/log" imported as "l" but must be "log" according to config (importas)
72-74: Improved error handling for missing componentsThe changes here implement the requested debug logging and user-friendly error message. However, there are two static analysis issues:
- Using dynamic errors (fmt.Errorf) instead of wrapped static errors
- Error message starts with capital letter
l.Debug(err) - u.PrintErrorMarkdownAndExit("", fmt.Errorf("Component not found"), "") + u.PrintErrorMarkdownAndExit("", fmt.Errorf("component not found"), "")Consider defining a constant for this error message to address the dynamic error issue.
🧰 Tools
🪛 GitHub Check: golangci
[failure] 73-73: [golangci] cmd/docs.go#L73
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("Component not found")" (err113)
76-76: Consistent error for missing componentSame static analysis issues as the previous error message.
- u.PrintErrorMarkdownAndExit("", fmt.Errorf("Component not found"), "") + u.PrintErrorMarkdownAndExit("", fmt.Errorf("component not found"), "")🧰 Tools
🪛 GitHub Check: golangci
[failure] 76-76: [golangci] cmd/docs.go#L76
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("Component not found")" (err113)
81-81: User-friendly message for missing READMEThis implements the requested feature to provide a helpful error message when documentation is missing. However, static analysis flags capitalization and punctuation in the error string.
- u.PrintErrorMarkdownAndExit("", fmt.Errorf("Documentation is missing for the component `%s`. Consider adding a README.md to provide more context and details.", info.Component), "") + u.PrintErrorMarkdownAndExit("", fmt.Errorf("documentation is missing for the component `%s`; consider adding a README.md to provide more context and details", info.Component), "")🧰 Tools
🪛 GitHub Check: golangci
[failure] 81-81: [golangci] cmd/docs.go#L81
error-strings: error strings should not be capitalized or end with punctuation or a newline (revive)
96-97: Consistent error format for markdown rendererGood improvement to error handling, but remember to follow error string formatting standards.
- u.PrintErrorMarkdownAndExit("", fmt.Errorf("failed to initialize markdown renderer: %w", err), "") + u.PrintErrorMarkdownAndExit("", fmt.Errorf("failed to initialize markdown renderer: %w", err), "")The format with a lowercase initial letter and %w error wrapping is correct here.
🛑 Comments failed to post (1)
cmd/docs.go (1)
72-82: 💡 Verification agent
❓ Verification inconclusive
Missing test case
In the PR comments, osterman requested adding a test case to validate these changes. Make sure to add a test that checks for a non-zero exit code and verifies the display of a helpful error message when documentation is not found.
🏁 Script executed:
#!/bin/bash # Check if test case was added as requested in PR comments grep -r "documentation is missing" "tests/test-cases"Length of output: 53
Action Required: Add Missing Test Case for Documentation Error
The updated code in
cmd/docs.go(lines 72-82) correctly exits when documentation is missing, but as noted, Osterman requested a test case to validate this behavior. Please add a test that:
- Invokes the scenario where the component’s README.md is missing.
- Verifies that the process exits with a non-zero exit code.
- Confirms that the error message (indicating that documentation is missing) is clearly displayed.
🧰 Tools
🪛 GitHub Check: golangci
[failure] 81-81: [golangci] cmd/docs.go#L81
error-strings: error strings should not be capitalized or end with punctuation or a newline (revive)
[failure] 73-73: [golangci] cmd/docs.go#L73
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("Component not found")" (err113)
[failure] 76-76: [golangci] cmd/docs.go#L76
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("Component not found")" (err113)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1032 +/- ##
=======================================
Coverage 17.44% 17.45%
=======================================
Files 170 170
Lines 18764 18762 -2
=======================================
Hits 3274 3274
+ Misses 14892 14890 -2
Partials 598 598
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🛑 Comments failed to post (3)
cmd/docs.go (3)
12-12:
⚠️ Potential issueAlias required for imported log package.
The
github.com/charmbracelet/logpackage should be imported with an alias according to project configuration.- "github.com/charmbracelet/log" + log "github.com/charmbracelet/log"📝 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."log" "github.com/charmbracelet/log"🧰 Tools
🪛 golangci-lint (1.62.2)
12-12: import "github.com/charmbracelet/log" imported without alias but must be with alias "log" according to config
(importas)
🪛 GitHub Check: golangci
[failure] 12-12: [golangci] cmd/docs.go#L12
import "github.com/charmbracelet/log" imported without alias but must be with alias "log" according to config (importas)
71-73: 🛠️ Refactor suggestion
Improved error handling with static errors.
Good job logging the actual error for debugging while showing a friendly message to users. However, the linter flags that dynamic errors should be replaced with wrapped static errors.
log.Debug(err) -u.PrintErrorMarkdownAndExit("", fmt.Errorf("Component not found"), "") +var ErrComponentNotFound = fmt.Errorf("component not found") +u.PrintErrorMarkdownAndExit("", ErrComponentNotFound, "")Move the static error declaration to the package level constants.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: golangci
[failure] 72-72: [golangci] cmd/docs.go#L72
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("Component not found")" (err113)
80-80: 🛠️ Refactor suggestion
Fix error message capitalization and formatting.
The linter is flagging that error strings should not be capitalized or end with punctuation. Let's fix the formatting while keeping the helpful message.
-u.PrintErrorMarkdownAndExit("", fmt.Errorf("Documentation is missing for the component `%s`. Consider adding a README.md to provide more context and details.", info.Component), "") +var ErrDocumentationMissing = fmt.Errorf("documentation is missing for the component `%s`; consider adding a README.md to provide more context and details") +u.PrintErrorMarkdownAndExit("", fmt.Errorf(ErrDocumentationMissing.Error(), info.Component), "")Move the static error declaration to the package level constants.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: golangci
[failure] 80-80: [golangci] cmd/docs.go#L80
error-strings: error strings should not be capitalized or end with punctuation or a newline (revive)
what
why
references
DEV-2718
Summary by CodeRabbit