-
Notifications
You must be signed in to change notification settings - Fork 24
recover excercise new pull req #109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdds a new exercise "Safe Panic Recovery" (slug Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Run as Run(n)
participant DoWork as DoWork(n)
Caller->>Run: call with n
activate Run
note right of Run #E6F7FF: deferred function calls recover()
Run->>DoWork: invoke DoWork(n)
alt n < 0
DoWork--x Run: panic("input cannot be negative")
note right of Run #FFF4E6: deferred recover captures panic value
else n >= 0
DoWork-->>Run: return normally
end
Run-->>Caller: return recoveredValue (nil or panic value)
deactivate Run
sequenceDiagram
autonumber
actor Client
participant Get as Get(slug)
participant Catalog as catalog.yaml
participant Local as local list
participant Templates as embedded templates/solutions
Client->>Get: request slug
Get->>Catalog: lookup slug
alt found in catalog
Catalog-->>Get: Exercise entry
else not in catalog
Get->>Local: lookup slug
alt found locally
Local-->>Get: Exercise entry
else not found locally
Get->>Templates: check embedded template/solution
alt template/solution exists
Templates-->>Get: synthesize Exercise (slug/title, test_regex ".*")
else none
Get-->>Client: not-found error
end
end
end
Get-->>Client: Exercise (entry or synthesized) / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
pls review now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - hint_type: concept | ||
| text: The `recover()` function is ONLY useful inside a function that is executed by a `defer` statement. Outside of a deferred function, `recover` will return `nil` and have no effect. | ||
| - hint_type: structure | ||
| text: You must place a `defer` statement at the beginning of the `Run` function. The function passed to `defer` is where you should call `recover()`. | ||
| - hint_type: solution | ||
| text: "The core logic of the deferred function should look like this: `if r := recover(); r != nil { recoveredValue = r }`" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restore string hints to keep catalog schema valid
Every existing catalog entry provides hints as plain strings. The loader expects a []string; supplying maps (hint_type, text) will fail to unmarshal and break catalog loading. Please keep hints as simple strings (or extend the loader before changing the schema).
- - hint_type: concept
- text: The `recover()` function is ONLY useful inside a function that is executed by a `defer` statement. Outside of a deferred function, `recover` will return `nil` and have no effect.
- - hint_type: structure
- text: You must place a `defer` statement at the beginning of the `Run` function. The function passed to `defer` is where you should call `recover()`.
- - hint_type: solution
- text: "The core logic of the deferred function should look like this: `if r := recover(); r != nil { recoveredValue = r }`"
+ - The `recover()` function is ONLY useful inside a function that is executed by a `defer` statement. Outside of a deferred function, `recover` will return `nil` and have no effect.
+ - Place a `defer` statement at the beginning of `Run`; call `recover()` inside the deferred function.
+ - The deferred function can follow: `if r := recover(); r != nil { recoveredValue = r }`📝 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.
| - hint_type: concept | |
| text: The `recover()` function is ONLY useful inside a function that is executed by a `defer` statement. Outside of a deferred function, `recover` will return `nil` and have no effect. | |
| - hint_type: structure | |
| text: You must place a `defer` statement at the beginning of the `Run` function. The function passed to `defer` is where you should call `recover()`. | |
| - hint_type: solution | |
| text: "The core logic of the deferred function should look like this: `if r := recover(); r != nil { recoveredValue = r }`" | |
| - The `recover()` function is ONLY useful inside a function that is executed by a `defer` statement. Outside of a deferred function, `recover` will return `nil` and have no effect. | |
| - Place a `defer` statement at the beginning of `Run`; call `recover()` inside the deferred function. | |
| - The deferred function can follow: `if r := recover(); r != nil { recoveredValue = r }` |
🤖 Prompt for AI Agents
In internal/exercises/catalog.yaml around lines 206 to 211, the hints are
defined as maps with keys like hint_type/text which breaks the loader expecting
a []string; replace those three map entries with simple string items containing
the hint text (i.e., convert each map to a plain string entry using the existing
text values) so the hints remain a list of strings and the catalog loader can
unmarshal them correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exercises/exercises.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/exercises/exercises.go (1)
internal/exercises/solutions.go (1)
SolutionExists(18-22)
| // Fallback: if an embedded template or solution exists, synthesize an Exercise entry | ||
| if templateExists(slug) || SolutionExists(slug) { | ||
| fmt.Fprintf(os.Stderr, | ||
| "Warning: exercise '%s' found in templates/solutions but missing from catalog.yaml\n", | ||
| slug, | ||
| ) | ||
| return Exercise{ | ||
| Slug: slug, | ||
| Title: formatSlugAsTitle(slug), // e.g., "110 Recover" | ||
| TestRegex: ".*", | ||
| Hints: []string{"This exercise is missing proper catalog metadata. Check documentation."}, | ||
| }, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Omesh2004 : Why do we need support this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The formatSlugAsTitle helper is only used in the fallback path when the slug isn’t in catalog.yaml (and not in local ./exercises) but exists in embedded templates/solutions..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nice edition, could you add this as separate PR?
| func formatSlugAsTitle(slug string) string { | ||
| s := strings.TrimSpace(slug) | ||
| if s == "" { | ||
| return "Exercise" | ||
| } | ||
| parts := strings.Split(s, "_") | ||
| for i, p := range parts { | ||
| if p == "" { | ||
| continue | ||
| } | ||
| // Keep purely numeric segments as-is (e.g., "110") | ||
| isDigits := true | ||
| for _, r := range p { | ||
| if r < '0' || r > '9' { | ||
| isDigits = false | ||
| break | ||
| } | ||
| } | ||
| if isDigits { | ||
| parts[i] = p | ||
| continue | ||
| } | ||
| upper := strings.ToUpper(p) | ||
| switch upper { | ||
| case "JSON", "XML", "HTTP", "CLI", "KV", "ID", "URL", "IO": | ||
| parts[i] = upper | ||
| default: | ||
| parts[i] = strings.ToUpper(p[:1]) + strings.ToLower(p[1:]) | ||
| } | ||
| } | ||
| return strings.Join(parts, " ") | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate what are we trying to achieve here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
basically this helper function is alloting titles based on the slugs in case the same arent present in catalog.yaml file
110_recover -> 110 Recover
36_json -> 36 JSON
104_http_server -> 104 HTTP Server
105_cli_todo_list -> 105 CLI Todo List are some of the examples for human readable titles
|
@Omesh2004 Please do not do multiple things in single PR, keep this PR focused on recover exercise. |
Summary
Describe the change and its motivation.
i have added the recover excercise without merge conflicts
Checklist
Screenshots / Output (if CLI UX)
Paste before/after where helpful.
Related issues
Fixes #
Summary by CodeRabbit