01 - Feat/defer exercises - #91
Conversation
WalkthroughIntroduces a new “Deferred Execution” concept and exercise (28_defer). Updates catalog entries, including adding the new concept and renumbering several project slugs. Adds template code, utilities, and tests for the defer exercise, plus a corresponding solution implementation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Test (defer_test.go)
participant U as CreateFile (utils.go)
participant W as WriteToFile (defer.go)
participant F as os.File
note over T: Setup
T->>U: CreateFile()
U-->>T: *os.File (singleton)
note over T,W: Exercise under test
T->>W: WriteToFile(f)
W->>U: CreateFile()
U-->>W: *os.File
rect rgba(230,245,255,0.6)
note right of W: defer f.Close()
end
W->>F: Fprintln("data"/"ABC")
W-->>T: return (deferred Close pending)
note over F: Close executes after return
rect rgba(255,235,230,0.6)
T->>F: Fprintln("again")
F-->>T: error (file closed)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
internal/exercises/templates/28_defer/defer.go (1)
10-10: Consider whether to handle the Fprintln error.
fmt.Fprintlnreturns an error that is currently ignored. For teaching purposes, you may want to either handle it or add a comment explaining why it's acceptable to ignore in this exercise context.internal/exercises/templates/28_defer/utils.go (1)
13-23: Singleton temporary file lacks explicit cleanup.The singleton pattern ensures a single shared file across all calls, which is appropriate for this exercise. However, the temporary file is never explicitly removed and will persist until program termination or OS cleanup.
For exercise purposes this is acceptable, but consider whether to add a cleanup function or document this behavior in a comment.
internal/exercises/solutions/28_defer/defer.go (2)
8-12: Defer usage is correct, but parameter remains unused.The solution correctly demonstrates
defer f.Close()for resource cleanup. However, like the template, the*os.Fileparameter is never used. Consider aligning the function signature with its actual usage.
10-10: Close() error is ignored.In production code, you should check the error returned by
Close(), especially after writes, as buffered data may fail to flush. For this teaching exercise, the simplified approach may be acceptable.Example of proper error handling:
defer func() { if err := f.Close(); err != nil { // Handle close error } }()internal/exercises/templates/28_defer/defer_test.go (1)
9-10: Clarify the unused parameter pattern.The test passes
ftoWriteToFile(f), butWriteToFileignores this parameter and callsCreateFile()internally. Since both return the same singleton instance, the test works, but the parameter passing is confusing.Consider either:
- Removing the parameter from
WriteToFileentirely- Having
WriteToFileuse the passed parameter instead of callingCreateFile()- Adding comments explaining the pedagogical reason for this design
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/exercises/catalog.yaml(1 hunks)internal/exercises/solutions/28_defer/defer.go(1 hunks)internal/exercises/templates/28_defer/defer.go(1 hunks)internal/exercises/templates/28_defer/defer_test.go(1 hunks)internal/exercises/templates/28_defer/utils.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
internal/exercises/solutions/28_defer/defer.go (2)
internal/exercises/templates/28_defer/defer.go (1)
WriteToFile(8-11)internal/exercises/templates/28_defer/utils.go (1)
CreateFile(14-23)
internal/exercises/templates/28_defer/defer_test.go (3)
internal/exercises/templates/28_defer/utils.go (1)
CreateFile(14-23)internal/exercises/solutions/28_defer/defer.go (1)
WriteToFile(8-12)internal/exercises/templates/28_defer/defer.go (1)
WriteToFile(8-11)
internal/exercises/templates/28_defer/defer.go (2)
internal/exercises/solutions/28_defer/defer.go (1)
WriteToFile(8-12)internal/exercises/templates/28_defer/utils.go (1)
CreateFile(14-23)
🔇 Additional comments (6)
internal/exercises/templates/28_defer/utils.go (1)
18-18: Panic is acceptable for exercise code.Using
panicfor file creation errors is appropriate for teaching exercises where simplicity is valued. In production code, you'd typically return the error to the caller.internal/exercises/templates/28_defer/defer_test.go (1)
8-17: Test correctly validates defer/close behavior.The test verifies that
WriteToFilecloses the file by attempting a write afterward and expecting an error. This effectively tests whether students have added thedefer f.Close()statement.internal/exercises/catalog.yaml (3)
137-141: New defer concept entry looks good.The new
28_deferconcept entry is well-structured with an appropriate hint pointing students toward usingdeferwithf.Close(). This aligns with the template and test implementations.
185-192: Expanded hints for epoch exercise.The
109_epochentry now includes helpful hints abouttime.Unix(),t.Unix(), andtime.Parse. These additions improve the exercise guidance.
144-184: All renamed project slugs verified. No remaining references to the old slugs were found; directory names and catalog.yaml entries correctly use slugs 101–109.internal/exercises/templates/28_defer/defer.go (1)
8-11: Clarify the unused*os.Fileparameter and adddefer f.Close().
- The function signature never uses its
*os.Fileargument (it always callsCreateFile()); remove the parameter or document its pedagogical intent.- After opening the file, students must defer
f.Close()—omitting it currently causes the test to fail.
|
@kaushalyap: Can you re-raise PR for this? Please join community to discuss and take this forward: https://join.ohmyscript.com Apologies for inconvenience here, needed to set the release cycle properly hence would be great if you could raise this again, I will let you know next steps accordingly |
|
@zhravan Okay I'll reraise it and join the community |
Summary
Describe the change and its motivation.
Checklist
make verifyorgolearn verify <slug>(CLI does not work as said in an issue, but tested locally usinggo test.)Screenshots / Output (if CLI UX)
Paste before/after where helpful.
Related issues
Fixes #75
Summary by CodeRabbit