feat(exercises): switch catalog.yaml to directory-based catalog loader #172
Conversation
|
Warning Rate limit exceeded@LeeFred3042U has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 13 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (64)
WalkthroughReplaces the single embedded Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Caller (ListAll / Init / Get / InitAll)
participant Loader as catalog loader (catalogLoader)
participant LocalFS as Local filesystem (./exercises)
participant EmbFS as Embedded FS (Catalog/**)
participant Copier as Template copier / Sandbox creator
Caller->>Loader: request catalog (ListAll/Get)
Loader->>LocalFS: discoverLocal()
alt local exercises found
LocalFS-->>Loader: local entries
Loader-->>Caller: return local-first entries
else
Loader->>EmbFS: loadCatalogFromFS()
EmbFS-->>Loader: YAML files per-exercise
Loader->>Loader: parse, validate, sort
Loader-->>Caller: return embedded catalog
end
Caller->>Copier: Init exercise / InitAll
Copier->>EmbFS: templateExists(templates/<ex.Path()>)
alt templates exist
EmbFS-->>Copier: template tree
Copier->>LocalFS: create dirs & write files (use ex.Path()/ex.Dir)
LocalFS-->>Copier: ack
Copier-->>Caller: init complete
else
Copier-->>Caller: ErrNoTemplates
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
internal/exercises/Catalog/Concepts/39_panic.yaml (1)
1-9: Pattern adherence is correct; consider removing trailing blank line.Slug-filename alignment is correct, YAML is valid. The four hints appropriately scaffold a complex topic (panic, defer, recover). Line 9 contains a trailing blank line that could be removed for consistency with other files.
internal/exercises/Catalog/Concepts/42_wait_group.yaml (1)
12-12: Minor: Remove trailing whitespace.Line 12 contains only whitespace. Consider removing it for cleaner formatting.
internal/exercises/Catalog/Concepts/45_range_over_channels.yaml (1)
5-5: Minor: Fix grammatical error in hint. "Range over a channels" should be "Range over channels" (no article, singular).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (62)
internal/exercises/Catalog/Concepts/01_hello.yaml(1 hunks)internal/exercises/Catalog/Concepts/02_values.yaml(1 hunks)internal/exercises/Catalog/Concepts/03_variables.yaml(1 hunks)internal/exercises/Catalog/Concepts/04_constants.yaml(1 hunks)internal/exercises/Catalog/Concepts/05_for.yaml(1 hunks)internal/exercises/Catalog/Concepts/06_if_else.yaml(1 hunks)internal/exercises/Catalog/Concepts/07_switch.yaml(1 hunks)internal/exercises/Catalog/Concepts/08_arrays.yaml(1 hunks)internal/exercises/Catalog/Concepts/09_slices.yaml(1 hunks)internal/exercises/Catalog/Concepts/10_maps.yaml(1 hunks)internal/exercises/Catalog/Concepts/11_functions.yaml(1 hunks)internal/exercises/Catalog/Concepts/12_multi_return.yaml(1 hunks)internal/exercises/Catalog/Concepts/13_variadic.yaml(1 hunks)internal/exercises/Catalog/Concepts/14_closures.yaml(1 hunks)internal/exercises/Catalog/Concepts/15_recursion.yaml(1 hunks)internal/exercises/Catalog/Concepts/16_range_built_in.yaml(1 hunks)internal/exercises/Catalog/Concepts/17_pointers.yaml(1 hunks)internal/exercises/Catalog/Concepts/18_strings_runes.yaml(1 hunks)internal/exercises/Catalog/Concepts/19_structs.yaml(1 hunks)internal/exercises/Catalog/Concepts/20_methods.yaml(1 hunks)internal/exercises/Catalog/Concepts/21_interfaces.yaml(1 hunks)internal/exercises/Catalog/Concepts/22_enums.yaml(1 hunks)internal/exercises/Catalog/Concepts/23_struct_embedding.yaml(1 hunks)internal/exercises/Catalog/Concepts/24_generics.yaml(1 hunks)internal/exercises/Catalog/Concepts/25_range_iterators.yaml(1 hunks)internal/exercises/Catalog/Concepts/26_errors.yaml(1 hunks)internal/exercises/Catalog/Concepts/27_custom_errors.yaml(1 hunks)internal/exercises/Catalog/Concepts/28_defer.yaml(1 hunks)internal/exercises/Catalog/Concepts/29_go_routines.yaml(1 hunks)internal/exercises/Catalog/Concepts/30_channels.yaml(1 hunks)internal/exercises/Catalog/Concepts/31_mutexes.yaml(1 hunks)internal/exercises/Catalog/Concepts/32_sorting.yaml(1 hunks)internal/exercises/Catalog/Concepts/33_string_formatting.yaml(1 hunks)internal/exercises/Catalog/Concepts/34_channel_buffering.yaml(1 hunks)internal/exercises/Catalog/Concepts/35_channel_sync.yaml(1 hunks)internal/exercises/Catalog/Concepts/36_json.yaml(1 hunks)internal/exercises/Catalog/Concepts/37_sorting_by_functions.yaml(1 hunks)internal/exercises/Catalog/Concepts/37_xml.yaml(1 hunks)internal/exercises/Catalog/Concepts/38_time_formatting.yaml(1 hunks)internal/exercises/Catalog/Concepts/39_channel_directions.yaml(1 hunks)internal/exercises/Catalog/Concepts/39_panic.yaml(1 hunks)internal/exercises/Catalog/Concepts/40_channel_select.yaml(1 hunks)internal/exercises/Catalog/Concepts/41_time_delay.yaml(1 hunks)internal/exercises/Catalog/Concepts/42_wait_group.yaml(1 hunks)internal/exercises/Catalog/Concepts/43_worker_pools.yaml(1 hunks)internal/exercises/Catalog/Concepts/44_atomic_counters.yaml(1 hunks)internal/exercises/Catalog/Concepts/45_range_over_channels.yaml(1 hunks)internal/exercises/Catalog/Concepts/46_non_blocking_channel_operations.yaml(1 hunks)internal/exercises/Catalog/Concepts/64_timers.yaml(1 hunks)internal/exercises/Catalog/Concepts/68_rate_limiting.yaml(1 hunks)internal/exercises/Catalog/Projects/01_text_analyzer.yaml(1 hunks)internal/exercises/Catalog/Projects/02_shape_calculator.yaml(1 hunks)internal/exercises/Catalog/Projects/03_task_scheduler.yaml(1 hunks)internal/exercises/Catalog/Projects/04_http_server.yaml(1 hunks)internal/exercises/Catalog/Projects/05_cli_todo_list.yaml(1 hunks)internal/exercises/Catalog/Projects/06_simple_chat_app.yaml(1 hunks)internal/exercises/Catalog/Projects/07_image_processing_utility.yaml(1 hunks)internal/exercises/Catalog/Projects/08_basic_key_value_store.yaml(1 hunks)internal/exercises/Catalog/Projects/09_epoch.yaml(1 hunks)internal/exercises/catalog.yaml(0 hunks)internal/exercises/exercises.go(9 hunks)internal/exercises/exercises_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- internal/exercises/catalog.yaml
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-02T12:51:43.433Z
Learnt from: kaushalyap
Repo: zhravan/golearn PR: 94
File: internal/exercises/templates/28_defer/utils.go:0-0
Timestamp: 2025-10-02T12:51:43.433Z
Learning: In the defer exercise template (internal/exercises/templates/28_defer/utils.go), the temporary file created by CreateFile() intentionally lacks cleanup (no Close() or Remove()) as part of the pedagogical design, expecting students to implement proper resource cleanup using defer.
Applied to files:
internal/exercises/Catalog/Concepts/39_panic.yamlinternal/exercises/Catalog/Concepts/41_time_delay.yamlinternal/exercises/Catalog/Concepts/28_defer.yamlinternal/exercises/exercises.go
📚 Learning: 2025-10-04T21:41:57.043Z
Learnt from: bdeb1337
Repo: zhravan/golearn PR: 132
File: internal/exercises/solutions/06_if_else/if_else.go:1-12
Timestamp: 2025-10-04T21:41:57.043Z
Learning: In the golearn repository, exercise solutions belong in `internal/exercises/solutions/$ex/` (reference implementations), while `internal/exercises/templates/$ex/` contains starter code with TODOs and test files for learners.
Applied to files:
internal/exercises/Catalog/Concepts/29_go_routines.yamlinternal/exercises/Catalog/Concepts/01_hello.yaml
📚 Learning: 2025-10-10T15:23:15.855Z
Learnt from: kaushalyap
Repo: zhravan/golearn PR: 156
File: internal/exercises/templates/40_channel_select/40_channel_select_test.go:10-24
Timestamp: 2025-10-10T15:23:15.855Z
Learning: In the golearn repository, timing-dependent tests in exercises (like the channel select exercise) don't need to meet production-grade reliability standards. Tests that may have some flakiness due to timing are acceptable if they effectively demonstrate the concept being taught.
Applied to files:
internal/exercises/Catalog/Concepts/41_time_delay.yaml
📚 Learning: 2025-10-02T06:30:47.152Z
Learnt from: kaushalyap
Repo: zhravan/golearn PR: 110
File: internal/exercises/templates/28_defer/defer.go:14-19
Timestamp: 2025-10-02T06:30:47.152Z
Learning: In the defer exercise template (internal/exercises/templates/28_defer/defer.go), the unused *os.File parameter in WriteToFile is intentional and part of the pedagogical design for teaching defer concepts.
Applied to files:
internal/exercises/Catalog/Concepts/28_defer.yaml
🧬 Code graph analysis (1)
internal/exercises/exercises_test.go (1)
internal/exercises/exercises.go (1)
Exercise(22-27)
🪛 GitHub Actions: CI
internal/exercises/exercises_test.go
[error] 1-1: Go formatting check failed: Code is not formatted. Run 'go fmt ./...' to fix.
🔇 Additional comments (79)
internal/exercises/Catalog/Concepts/06_if_else.yaml (1)
1-5: Structure and pattern look good.Slug-filename alignment is correct, YAML is valid, and the hint is appropriate for the exercise.
internal/exercises/Catalog/Concepts/11_functions.yaml (1)
1-5: Consistent with established pattern.Slug alignment and YAML structure are correct. Hint is clear and guides students appropriately.
internal/exercises/Catalog/Concepts/19_structs.yaml (1)
1-5: Pattern adherence is solid.Slug-filename consistency maintained, YAML valid, hint is concrete and actionable.
internal/exercises/Catalog/Concepts/18_strings_runes.yaml (1)
1-5: Valid structure and consistent naming.Slug aligns with filename; YAML is well-formed. Hint is detailed and covers both iteration and manipulation aspects.
internal/exercises/Catalog/Concepts/05_for.yaml (1)
1-5: Pattern is consistent.Slug-filename correspondence correct, YAML valid, hint is focused on core for-loop use case (accumulation).
internal/exercises/Catalog/Projects/05_cli_todo_list.yaml (1)
1-5: Project definition follows pattern.Slug-filename alignment correct, YAML valid. The "(Medium)" difficulty label in the title is appropriate for projects and differs from concept entries; appears to be an intentional convention.
internal/exercises/Catalog/Concepts/32_sorting.yaml (1)
1-5: Consistent with catalog pattern.Slug-filename alignment correct, YAML valid, hint properly references Go API using code formatting.
internal/exercises/Catalog/Concepts/25_range_iterators.yaml (1)
1-5: LGTM — well-formed exercise descriptor with clear, actionable hint.internal/exercises/Catalog/Concepts/27_custom_errors.yaml (1)
1-5: LGTM — well-formed descriptor with concrete guidance on custom error implementations.internal/exercises/Catalog/Concepts/09_slices.yaml (1)
1-5: LGTM — well-structured descriptor with focused hint on slice operations.internal/exercises/Catalog/Concepts/16_range_built_in.yaml (1)
1-5: LGTM — descriptor clearly targets iteration over collections using range.internal/exercises/Catalog/Concepts/01_hello.yaml (1)
1-5: LGTM — appropriate introductory exercise with clear expected outcome.internal/exercises/Catalog/Concepts/30_channels.yaml (1)
1-5: LGTM — descriptor provides concrete syntax guidance for Go channels.internal/exercises/Catalog/Concepts/23_struct_embedding.yaml (1)
1-5: LGTM — descriptor covers embedding pattern with clear guidance on field access.internal/exercises/Catalog/Concepts/12_multi_return.yaml (1)
1-5: LGTM — descriptor provides concrete use case with specific return requirements.internal/exercises/Catalog/Concepts/17_pointers.yaml (1)
1-5: LGTM!Valid YAML structure with clear exercise metadata. Slug naming and hint content are appropriate for the pointers concept.
internal/exercises/Catalog/Concepts/21_interfaces.yaml (1)
1-5: LGTM!Valid YAML structure with clear exercise metadata for the interfaces concept. Slug and hint are appropriate.
internal/exercises/Catalog/Concepts/24_generics.yaml (1)
1-5: LGTM!Valid YAML structure with clear exercise metadata. Slug and hint content are appropriate for generics concept.
internal/exercises/Catalog/Concepts/35_channel_sync.yaml (1)
1-5: LGTM!Valid YAML structure with appropriate exercise metadata. Minor note: hint uses "go routine" instead of "goroutine" (Go convention), but this is a minor user-facing text issue.
internal/exercises/Catalog/Projects/09_epoch.yaml (1)
1-9: LGTM!Valid project exercise definition with appropriate metadata. The expanded structure (difficulty, topics, multiple hints) is well-suited for project-level exercises. All hints are clear and provide actionable guidance.
internal/exercises/Catalog/Concepts/02_values.yaml (1)
1-5: LGTM!Valid YAML structure with clear exercise metadata. Slug naming and hint are appropriate for the values concept.
internal/exercises/Catalog/Concepts/15_recursion.yaml (1)
1-5: LGTM!Valid YAML structure with clear exercise metadata. Slug and hint content are appropriate for the recursion concept.
internal/exercises/Catalog/Concepts/46_non_blocking_channel_operations.yaml (1)
1-7: LGTM!Valid YAML structure with clear exercise metadata. Multiple hints with inline code formatting (backticks) are well-presented and provide actionable guidance for the non-blocking channel operations concept.
internal/exercises/Catalog/Concepts/28_defer.yaml (1)
1-5: LGTM!Minimal, well-formed YAML descriptor with consistent structure. Slug, title, test_regex, and hint align with the exercise.
internal/exercises/Catalog/Projects/01_text_analyzer.yaml (1)
1-5: LGTM!Well-formed project exercise descriptor with consistent structure and clear, actionable hints.
internal/exercises/Catalog/Concepts/31_mutexes.yaml (1)
1-5: LGTM!Concise, well-structured exercise descriptor. Consistent indentation and style with other minimal catalog entries.
internal/exercises/Catalog/Projects/06_simple_chat_app.yaml (1)
1-5: LGTM!Clear project exercise descriptor with actionable hints. Consistent formatting throughout.
internal/exercises/Catalog/Projects/08_basic_key_value_store.yaml (1)
1-5: LGTM!Well-defined project exercise with comprehensive hint on persistence patterns. Consistent structure and style.
internal/exercises/Catalog/Concepts/22_enums.yaml (1)
1-5: LGTM!Minimal, well-formed descriptor with clear pedagogical intent. Consistent with other concept exercise entries.
internal/exercises/Catalog/Projects/02_shape_calculator.yaml (1)
1-5: Verify optional fields are intentionally omitted.This exercise lacks
difficultyandtopicsmetadata fields that are present in other catalog entries (e.g., 41_time_delay.yaml, 42_wait_group.yaml). Confirm whether these fields should be standardized across all exercises or if they are truly optional. If optional, document this convention for future contributors.internal/exercises/Catalog/Concepts/33_string_formatting.yaml (1)
1-5: Align with optional field conventions.Like 02_shape_calculator.yaml, this entry omits
difficultyandtopics. Ensure the catalog loading mechanism and migration path are documented to clarify whether these are truly optional or should be standardized.internal/exercises/Catalog/Concepts/44_atomic_counters.yaml (1)
1-7: Clarify optional field standardization.This entry omits
difficultyandtopicsfields present in other exercises (e.g., 68_rate_limiting.yaml). Before merging, confirm whether these fields should be added here or whether the inconsistency is acceptable by design.internal/exercises/Catalog/Concepts/39_channel_directions.yaml (1)
1-9: Standardize optional field usage across exercises.This exercise, like 33_string_formatting.yaml and others, lacks
difficultyandtopics, while 41_time_delay.yaml and 68_rate_limiting.yaml include them. Document the expected fields or apply them consistently across all catalog entries.internal/exercises/Catalog/Concepts/41_time_delay.yaml (1)
1-10: Good: Enriched metadata improves exercise discovery.This entry includes
difficultyandtopicsmetadata (lines 3-4), which enhances exercise categorization. Consider adopting this pattern for all catalog entries to enable better filtering and organization.Verify whether the catalog loader supports and processes these optional fields, and whether all exercises should include them for consistency.
internal/exercises/Catalog/Concepts/03_variables.yaml (1)
1-5: Apply consistent metadata across catalog.This minimal entry lacks
difficultyandtopicsfields. Given that other exercises include these, establish a standard: either all exercises should include optional fields, or the loading mechanism should be documented to clarify which fields are required vs. optional.internal/exercises/Catalog/Concepts/42_wait_group.yaml (1)
1-11: Good: Comprehensive metadata and well-structured hints.This exercise properly includes
difficulty,topics, and detailed hints covering key WaitGroup patterns (Add/Done semantics, channel closing, loop variable capture). This is a good model for other exercises. Consider standardizing this enriched format across all catalog entries.internal/exercises/Catalog/Concepts/68_rate_limiting.yaml (1)
1-13: Excellent: Detailed exercise definition with clear implementation guidance.This exercise exemplifies the enriched metadata pattern with
difficulty: medium,topics, and seven well-structured hints covering struct design, concurrency, edge cases, testing concerns, and initialization. Use this as a model for standardizing other catalog entries.internal/exercises/Catalog/Concepts/08_arrays.yaml (1)
1-5: YAML structure is valid and complete.The exercise definition follows the established catalog format with slug, title, test_regex, and hints properly defined. The slug matches the filename convention.
internal/exercises/Catalog/Concepts/20_methods.yaml (1)
1-5: Valid exercise definition with consistent structure.The file follows the catalog format correctly. The hint appropriately guides learners toward implementing methods on structs.
internal/exercises/Catalog/Concepts/14_closures.yaml (1)
1-5: Valid catalog entry with appropriate closure exercise guidance.The structure and content conform to the established catalog format.
internal/exercises/Catalog/Concepts/26_errors.yaml (1)
1-5: Valid catalog entry for error handling concepts.The file structure is correct and the hint provides clear guidance for the errors exercise.
internal/exercises/Catalog/Concepts/37_xml.yaml (1)
1-8: Well-structured multi-hint exercise definition.The XML exercise appropriately includes multiple hints that progressively guide learners through encoding/decoding concepts, struct tags, and package usage.
internal/exercises/Catalog/Concepts/04_constants.yaml (1)
1-5: Valid constants exercise definition.The file follows established format with an appropriate hint for the constants concept.
internal/exercises/Catalog/Concepts/10_maps.yaml (1)
1-5: Valid maps exercise definition.The exercise provides practical guidance on using maps and string operations.
internal/exercises/Catalog/Concepts/40_channel_select.yaml (1)
1-6: Valid channel select exercise definition.The hints appropriately guide learners through concurrent channel operations and timeout patterns using Go's select and time constructs.
internal/exercises/Catalog/Concepts/34_channel_buffering.yaml (1)
1-5: LGTM. YAML structure is valid and complete.internal/exercises/Catalog/Projects/04_http_server.yaml (1)
1-5: LGTM. Structure and content are consistent.internal/exercises/Catalog/Concepts/29_go_routines.yaml (1)
1-5: LGTM. YAML structure and content are consistent.internal/exercises/Catalog/Projects/03_task_scheduler.yaml (1)
1-5: LGTM. Well-structured with comprehensive hint covering multiple concepts.internal/exercises/Catalog/Concepts/36_json.yaml (1)
1-9: LGTM. Multi-hint structure well-suited for this complex concept.internal/exercises/Catalog/Projects/07_image_processing_utility.yaml (1)
1-5: LGTM. Clear scope and difficulty level indicated.internal/exercises/Catalog/Concepts/13_variadic.yaml (1)
1-5: LGTM. Structure and content are consistent across the catalog.internal/exercises/Catalog/Concepts/07_switch.yaml (1)
1-5: LGTM! Well-formed exercise definition.The YAML structure correctly defines the switch exercise with all required fields. The hint is clear and relevant.
internal/exercises/Catalog/Concepts/43_worker_pools.yaml (1)
1-10: LGTM! Comprehensive worker pools exercise.The exercise definition is well-structured with detailed, pedagogically sound hints that cover all key aspects of the worker pool pattern in Go.
internal/exercises/Catalog/Concepts/64_timers.yaml (1)
6-15: LGTM! Comprehensive timer implementation hints.The hints provide excellent guidance covering timer management, concurrency safety, and proper resource cleanup.
internal/exercises/exercises_test.go (9)
12-16: Helper function is acceptable for test code.The
y()helper ignores the error fromyaml.Marshal, which is acceptable in test code since:
- It's only used with valid structs in controlled test scenarios
- Any malformed YAML will cause test failures downstream
- This pattern simplifies test data setup
19-56: LGTM! Well-structured test for YAML loading.The test correctly validates that
loadExercisesDirloads multiple YAML files and extracts the slugs without enforcing ordering (which is handled byloadCatalogFromFS).
59-70: LGTM! Proper validation of required slug field.The test correctly verifies that exercises without a slug are rejected with an appropriate error message.
72-85: LGTM! Proper handling of non-YAML files.The test correctly verifies that non-YAML files in the catalog directory are ignored during loading.
88-117: LGTM! Comprehensive test of catalog loading and sorting.The test correctly validates that
loadCatalogFromFSloads both concepts and projects and sorts them deterministically by slug.
119-136: LGTM! Proper handling of missing directories.The test correctly verifies that missing catalog directories (e.g., Projects) are gracefully handled without errors.
138-149: LGTM! Proper error handling for invalid YAML.The test correctly verifies that malformed YAML files are rejected with an error.
152-160: LGTM! Validates fallback catalog behavior.The test correctly verifies that the fallback catalog provides a minimal working exercise (01_hello) when loading fails.
186-191: LGTM! Proper handling of missing local directory.The test correctly verifies that
discoverLocal()doesn't error when the exercises directory is missing, which is expected behavior.internal/exercises/exercises.go (15)
10-10: LGTM! Necessary import for deterministic sorting.The
sortimport is correctly added to support sorting exercises by slug inloadCatalogFromFS.
19-19: LGTM! Embed directive updated for directory-based catalog.The embed directive correctly points to the new
Catalog/**directory structure, replacing the singlecatalog.yamlfile.
29-32: LGTM! Catalog struct correctly updated for programmatic loading.Removing the YAML tags is correct since the Catalog is now constructed programmatically in
loadCatalogFromFSrather than unmarshaled directly from YAML.
39-52: LGTM! Robust catalog loading with fallback.The singleton loader correctly falls back to a minimal catalog when loading fails or returns empty results, ensuring the system always has at least one exercise available.
54-75: LGTM! Well-implemented catalog loader with proper error handling.The function correctly:
- Loads from both Concepts and Projects directories
- Tolerates missing directories (returns empty slices)
- Sorts by slug for deterministic ordering
- Only propagates real errors (not "not exist")
77-115: LGTM! Robust directory loader with proper validation.The function correctly:
- Reads YAML files from a directory
- Skips non-YAML files and directories
- Supports both .yaml and .yml extensions
- Validates required fields (slug)
- Provides contextual error messages
117-128: LGTM! Appropriate fallback catalog.The fallback catalog provides a minimal working exercise (01_hello) to ensure the system remains functional even if catalog loading fails.
132-154: LGTM! Local discovery logic remains correct.The function properly discovers local exercises and gracefully handles missing directories, consistent with the test coverage.
156-169: LGTM! ListAll maintains expected behavior.The function correctly prioritizes local exercises over the catalog, preserving the existing public API behavior.
171-197: LGTM! Get function correctly searches all exercise sources.The function properly searches the catalog (both Concepts and Projects) and local exercises, maintaining the existing public API contract.
199-206: LGTM! Reset validates template existence.The function correctly checks that the template exists in the embedded FS before attempting to copy, providing clear error messages for unsupported exercises.
208-214: LGTM! Simple and correct template existence check.The function properly verifies template existence in the embedded FS using
fs.Stat.
232-270: LGTM! Robust template copying implementation.The function correctly:
- Creates a clean slate by removing existing directories
- Walks the template directory structure
- Preserves directory hierarchy
- Copies files with appropriate permissions (0o755 for dirs, 0o644 for files)
39-270: Excellent refactoring that achieves PR objectives.The migration from single-file
catalog.yamlto directory-based catalog loading is well-executed:
- Eliminates merge conflicts by splitting exercises into separate files
- Maintains backward compatibility with existing public APIs
- Includes robust fallback mechanisms
- Ensures deterministic ordering
The implementation follows Go best practices with clean separation of concerns and proper error handling.
216-230: Verify that all catalog exercises have corresponding templates.The function correctly initializes both Concepts and Projects. However, it will fail if any exercise in the catalog doesn't have a matching template in the embedded FS. Ensure all catalog YAML files have corresponding template directories.
internal/exercises/Catalog/Concepts/37_sorting_by_functions.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/exercises/exercises_test.go (1)
158-177: Test no longer exercisescatalog()and can dropcatalogOnceresetThis test now calls
loadCatalogFromFSdirectly, so:
- The name
TestCatalog_UsesDirectoryLoaderis slightly misleading.- The
catalogOnce = sync.Once{}assignment on Line 160 appears unused, since nothing in this test depends oncatalog()or the global once.I’d suggest either (a) renaming the test to reflect that it’s a direct
loadCatalogFromFScheck, or (b) refactoring production code so thatcatalog()can be exercised here via an injectable FS. In both cases, removing thecatalogOncereset will simplify the test and avoid unnecessary global mutation.
🧹 Nitpick comments (5)
internal/exercises/exercises_test.go (5)
57-68: Error assertion could be more robust than string matchingUsing
strings.Contains(err.Error(), "missing slug")works but couples the test to the exact wording. If you expose or already have a sentinel error for this case, consider asserting on that instead; otherwise this is acceptable.
70-83: Non-YAML filtering covered; optional stronger assertionThe test correctly verifies non-YAML files are ignored. You might also assert
items[0].Slug == "01_x"to make it explicit that the YAML entry is the one returned, but the length check already does most of the work.
85-114: Good coverage of concepts/projects loading and sort orderThis test nicely validates that both concepts and projects are loaded and that concepts are sorted by slug. If you ever suspect sort issues for projects too, adding a second project entry here would give symmetric coverage, but not strictly required.
135-146: Invalid YAML path exercised; could document error shapeEnsuring
loadCatalogFromFSreturns a non-nil error for malformed YAML is good. If you settle on a stable error prefix or wrapper for catalog load failures, you could assert on that substring to document the contract, but the current check is fine.
179-184: Test name suggests missing-dir case but setup doesn’t enforce itThe test only asserts that
discoverLocal()does not return an error in the current working directory; it doesn’t guarantee that the exercises directory is actually missing. If you specifically want to lock in the “no dir” behavior, consider callingdiscoverLocal()from withint.TempDir()(or another known-empty location) so the scenario is explicit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exercises/exercises_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/exercises/exercises_test.go (1)
internal/exercises/exercises.go (1)
Exercise(22-27)
🔇 Additional comments (4)
internal/exercises/exercises_test.go (4)
1-16: Top-level setup and helper read cleanlyPackage, imports, and the small
yhelper keep the tests concise and focused; no changes needed here.
18-55: Happy-path directory load test is well-shapedThe MapFS setup plus slug presence checks exercise
loadExercisesDirnicely without over-constraining ordering; this is a solid baseline test.
116-133: Missing catalog subdirectory behavior is clearly specifiedThis test makes it explicit that missing
Projectsdoes not error and yields zero projects while still loading concepts, which matches the intended tolerant behavior.
148-156: Fallback catalog behavior is well-pinnedAsserting that the fallback has exactly one concept with slug
01_hellonicely preserves the historical behavior when directory loading fails; no changes needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/exercises/exercises.go (3)
34-40: Catalog FS loader and fallback behavior look solidThe new
defaultCatalogLoader+catalog()+loadCatalogFromFSpipeline cleanly handles missingCatalog/Concepts/Catalog/Projectsdirs, enforces deterministic slug ordering, and falls back tofallbackCatalog()when loading fails or yields an empty catalog; this is a nice, robust upgrade over single-file YAML. One minor improvement to consider is surfacing/logging the underlying load error when you hit the fallback path so configuration issues don’t silently degrade to the hardcoded01_hellocatalog.Also applies to: 76-86, 88-109, 151-162
164-188: Confirm intended precedence and shape for locals vs catalog inListAll/Get
ListAllnow fully prefers discovered local exercises (returned asConceptsonly) whenever any local dirs exist, whileGetprioritizes catalog concepts/projects and only falls back to locals if the slug isn’t in the catalog. That asymmetry seems deliberate (per PR description: keep catalog behavior for Get/Init while exposing locals via ListAll), but it does mean you lose the concepts/projects distinction for locals and you can’t override catalog metadata via a local dir with the same slug. Please confirm this matches the CLI expectations; if you ever want local overrides for metadata too, you could invert theGetsearch order or merge locals into the catalog slices.Also applies to: 190-202, 205-230
232-239: Template reset/init behavior is consistent but stricter; verify missing-template handlingGuarding
ResetwithtemplateExistsis a nice UX improvement for local-only or ad‑hoc exercises, andcopyExerciseTemplate’s walk/relpath logic for copying embedded templates intoexercises/<slug>looks correct. In contrast,InitAllassumes every catalog concept/project has an embedded template and will fail fast if any template directory is missing; if that’s intentional (to catch catalog/template drift early), great—otherwise you may want to skip missing templates or return a sentinel likeErrNoTemplatesinstead. Also, ifErrNoTemplatesis no longer used anywhere, consider removing it to avoid dead code.Also applies to: 241-245, 247-260, 262-299, 301-301
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/exercises/exercises.go(11 hunks)internal/exercises/exercises_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exercises/exercises_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-02T12:51:43.433Z
Learnt from: kaushalyap
Repo: zhravan/golearn PR: 94
File: internal/exercises/templates/28_defer/utils.go:0-0
Timestamp: 2025-10-02T12:51:43.433Z
Learning: In the defer exercise template (internal/exercises/templates/28_defer/utils.go), the temporary file created by CreateFile() intentionally lacks cleanup (no Close() or Remove()) as part of the pedagogical design, expecting students to implement proper resource cleanup using defer.
Applied to files:
internal/exercises/exercises.go
🪛 GitHub Actions: CI
internal/exercises/exercises.go
[error] 1-1: Code is not formatted. Run 'go fmt ./...' to fix.
🔇 Additional comments (2)
internal/exercises/exercises.go (2)
111-149:loadExercisesDiris defensive and well-scopedDirectory scanning, YAML filtering, and validation all look good: non-YAML entries are ignored, YAML errors are wrapped with the filename, and missing
slugis treated as a hard error, which should make catalog issues easy to diagnose in tests. No changes needed here.
43-46: TightenwithTestCatalogLoaderand address formatting issuesThe override/reset logic for
defaultCatalogLoaderandcatalogOnceshould be consolidated into a single helper usingdeferfor safer restoration and clearer intent. Additionally, document that tests using this helper must not run in parallel. If true concurrent safety is needed in the future, consider makingcatalog()also respectcatalogMu.Run
go fmt ./internal/exercisesto fix formatting issues reported by CI.
…ises/exercises_test.go
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
internal/exercises/exercises.go (5)
34-72: StrengthenwithTestCatalogLoaderaround panics and potential racesThe loader indirection and test hook are a nice pattern, but two robustness points are worth considering:
- If
fn()panics, the restoration block is never reached, sodefaultCatalogLoader/catalogOnce/catalogDatastay in their test state and may affect later tests.catalog()readsdefaultCatalogLoader/catalogDatawithoutcatalogMu, so ifwithTestCatalogLoaderis ever used from tests that run in parallel with other code callingcatalog(), the writes in this helper can technically race with reads incatalog().A minimal improvement that doesn’t change semantics would be to use
deferfor restoration and document that this helper must not be used from tests that callt.Parallel()or otherwise accesscatalog()concurrently with it:func withTestCatalogLoader(loader func() (Catalog, error), fn func()) { - catalogMu.Lock() - - oldLoader := defaultCatalogLoader - - // override loader + reset singleton - defaultCatalogLoader = loader - catalogOnce = sync.Once{} - catalogData = Catalog{} - - catalogMu.Unlock() - - fn() - - // restore loader, and reset the once/data again - catalogMu.Lock() - defaultCatalogLoader = oldLoader - catalogOnce = sync.Once{} - catalogData = Catalog{} - catalogMu.Unlock() + catalogMu.Lock() + oldLoader := defaultCatalogLoader + defaultCatalogLoader = loader + catalogOnce = sync.Once{} + catalogData = Catalog{} + catalogMu.Unlock() + + defer func() { + catalogMu.Lock() + defaultCatalogLoader = oldLoader + catalogOnce = sync.Once{} + catalogData = Catalog{} + catalogMu.Unlock() + }() + + fn() }If you expect heavy parallelism in tests, you may also want to guard reads in
catalog()with the same mutex or switch to anatomic.Value-style pattern for the loader and data.Also applies to: 77-87
168-192: ConfirmListAllsemantics when any local exercises exist
ListAllnow returns only local exercises (mapped asConcepts) when there is at least one directory under./exercises, and completely hides embedded catalog concepts/projects in that case.This means that after initializing a single exercise locally,
ListAllwill stop showing all the other embedded catalog entries. If the CLI is intended to act as a “show me everything I could work on” view, you might instead want to merge embedded and local or at least make this precedence explicit in docs/UX.If “locals only” is the intended behavior (a “workspace view”), it would be good to codify that expectation in a comment or docstring.
Also applies to: 194-206
208-234:Getprefers embedded metadata over local; validate this precedence
Getsearches embedded concepts, then embedded projects, then locals. So if a slug exists both in the catalog and as a local directory (which will be the common case after an init), you always return the embedded metadata, not a local override.That’s consistent with keeping catalog as the source of truth, but it’s slightly asymmetric with
ListAll, which prefers local when any exist. If you want local exercises to be able to override catalog metadata (e.g., experimenting with different titles/test regexes), you might want to flip the order and search locals first.If catalog‑first is intentional, consider a short comment here to make that precedence explicit for future maintainers.
236-243:InitAllbehavior on missing templates
Resetguards withtemplateExists, butInitAllcallscopyExerciseTemplatefor every catalog entry without that check. That’s fine if you guarantee that every catalog exercise has a corresponding directory undertemplates/, but it means:
InitAllwill fail on the first exercise that lacks a template (due toWalkDir’s error), even if others are valid.- There’s no way to distinguish “some templates missing” from other IO errors.
If partial initialization is acceptable, you might consider skipping entries without templates (using
templateExists) and possibly returningErrNoTemplatesonly when nothing was initialized; otherwise, the current “fail fast on any missing template” behavior is clear but a bit strict.Also applies to: 251-264
266-303: Template copying is correct; consider preserving original file modesThe
copyExerciseTemplatelogic (clean target dir,WalkDirovertemplates/<slug>, create dirs, then copy files) looks correct and will produce the expected tree in./exercises/<slug>.Two minor improvement ideas, depending on how templates evolve:
- You currently write all files with
0o644. If any template files are intended to be executable (scripts, tools), you’ll lose the exec bit. You could instead read the originalfs.FileInfomode and propagate relevant bits.- You ignore errors from
os.RemoveAll(targetDir). That’s usually fine because subsequentMkdirAll/writes will still fail if there’s a real problem, but if you ever need more precise diagnostics, capturing that error (and failing early) could make debugging easier.Functionally this is fine as-is; these would just tighten the ergonomics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/exercises/exercises.go(10 hunks)internal/exercises/exercises_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exercises/exercises_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-02T12:51:43.433Z
Learnt from: kaushalyap
Repo: zhravan/golearn PR: 94
File: internal/exercises/templates/28_defer/utils.go:0-0
Timestamp: 2025-10-02T12:51:43.433Z
Learning: In the defer exercise template (internal/exercises/templates/28_defer/utils.go), the temporary file created by CreateFile() intentionally lacks cleanup (no Close() or Remove()) as part of the pedagogical design, expecting students to implement proper resource cleanup using defer.
Applied to files:
internal/exercises/exercises.go
🔇 Additional comments (2)
internal/exercises/exercises.go (2)
19-32: Catalog struct + embedded FS + fallback behavior look consistentEmbedding
Catalog/**, loading into the exportedCatalog{Concepts, Projects}fields, and falling back to a minimal in‑code catalog (01_hello) when nothing is found or loading fails gives you a robust default and matches the PR objective of decoupling from a monolithic YAML file. I don’t see functional issues with this struct layout or the fallback behavior as written.Also applies to: 154-166
10-10: FS-based catalog loader is correct and nicely constrained
loadCatalogFromFS+loadExercisesDirlook solid:
- Gracefully handle missing
Catalog/ConceptsorCatalog/Projectsby treatingfs.ErrNotExistas “no entries” instead of an error.- Filter to
.yaml/.ymlonly and ignore other files.- Fail fast on unreadable files, YAML errors, or missing
slug, with helpful error messages that point to the filename.- Sort concepts/projects by
Slugusingsort.Slicefor deterministic ordering.I don’t see correctness or performance issues here for the expected catalog sizes.
Also applies to: 89-152
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
internal/exercises/Catalog/Concepts/37_sorting_by_functions.yaml(1 hunks)internal/exercises/Catalog/Concepts/38_time_formatting.yaml(1 hunks)internal/exercises/Catalog/Concepts/39_panic.yaml(1 hunks)internal/exercises/Catalog/Concepts/41_time_delay.yaml(1 hunks)internal/exercises/Catalog/Concepts/42_wait_group.yaml(1 hunks)internal/exercises/Catalog/Concepts/64_timers.yaml(1 hunks)internal/exercises/Catalog/Concepts/68_rate_limiting.yaml(1 hunks)internal/exercises/Catalog/Projects/09_epoch.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/exercises/Catalog/Concepts/68_rate_limiting.yaml
- internal/exercises/Catalog/Concepts/37_sorting_by_functions.yaml
- internal/exercises/Catalog/Concepts/64_timers.yaml
- internal/exercises/Catalog/Concepts/38_time_formatting.yaml
- internal/exercises/Catalog/Concepts/41_time_delay.yaml
- internal/exercises/Catalog/Projects/09_epoch.yaml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bdeb1337
Repo: zhravan/golearn PR: 132
File: internal/exercises/solutions/06_if_else/if_else.go:1-12
Timestamp: 2025-10-04T21:41:57.043Z
Learning: In the golearn repository, exercise solutions belong in `internal/exercises/solutions/$ex/` (reference implementations), while `internal/exercises/templates/$ex/` contains starter code with TODOs and test files for learners.
📚 Learning: 2025-10-02T12:51:43.433Z
Learnt from: kaushalyap
Repo: zhravan/golearn PR: 94
File: internal/exercises/templates/28_defer/utils.go:0-0
Timestamp: 2025-10-02T12:51:43.433Z
Learning: In the defer exercise template (internal/exercises/templates/28_defer/utils.go), the temporary file created by CreateFile() intentionally lacks cleanup (no Close() or Remove()) as part of the pedagogical design, expecting students to implement proper resource cleanup using defer.
Applied to files:
internal/exercises/Catalog/Concepts/42_wait_group.yaml
🪛 YAMLlint (1.37.1)
internal/exercises/Catalog/Concepts/39_panic.yaml
[error] 8-8: syntax error: found character '`' that cannot start any token
(syntax)
🔇 Additional comments (1)
internal/exercises/Catalog/Concepts/42_wait_group.yaml (1)
1-9: YAML structure and pedagogical content are sound; verify supporting exercise files exist.The YAML definition is valid with correct indentation, all required fields (slug, title, test_regex, hints), and well-targeted hints addressing WaitGroup fundamentals, cleanup patterns, loop variable capture, and ordering expectations. Slug naming and title are clear.
Verification needed:
- Template file at
internal/exercises/templates/42_wait_group/with starter code (per learnings, templates contain TODOs for learners)- Solution file at
internal/exercises/solutions/42_wait_group/with reference implementation- Test coverage in
exercises_test.gothat includes this exercise in catalog loading tests- Slug sequence number consistency (verify
42is correct in ordering)These cannot be verified without repository access, but the YAML file itself requires no corrections.
|
@kaushalyap help |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: bdeb1337
Repo: zhravan/golearn PR: 132
File: internal/exercises/solutions/06_if_else/if_else.go:1-12
Timestamp: 2025-10-04T21:41:57.043Z
Learning: In the golearn repository, exercise solutions belong in `internal/exercises/solutions/$ex/` (reference implementations), while `internal/exercises/templates/$ex/` contains starter code with TODOs and test files for learners.
🪛 actionlint (1.7.9)
.github/workflows/ci.yml
26-26: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/exercises/solutions.go (2)
54-73: Address suppressed errors in file operations.Multiple file operation errors are silently ignored:
- Line 65:
filepath.Relerror suppressed (acceptable with fallback on line 230)- Line 67:
os.MkdirAllerror suppressed- Line 68:
fs.ReadFileerror suppressedIf
ReadFilefails, an empty or partial file will be written to disk without any indication of failure. Similarly, ifMkdirAllfails, the subsequentWriteFilewill fail anyway, but the error context will be lost.Apply this diff to improve error handling:
- rel, _ := filepath.Rel(templateRoot, path) + rel, err := filepath.Rel(templateRoot, path) + if err != nil { + return err + } dest := filepath.Join(workDir, rel) - _ = os.MkdirAll(filepath.Dir(dest), 0o755) - data, _ := fs.ReadFile(templatesFS, path) + if err := os.MkdirAll(filepath.Dir(dest), 0o755); err != nil { + return err + } + data, err := fs.ReadFile(templatesFS, path) + if err != nil { + return err + } return os.WriteFile(dest, data, 0o644)
77-96: Address suppressed errors in solution file operations.Similar to the template copying above, file operation errors are silently suppressed on lines 88, 90, and 91. This can lead to corrupted sandbox state without clear error messages.
Apply this diff to propagate errors properly:
- rel, _ := filepath.Rel(solutionRoot, path) + rel, err := filepath.Rel(solutionRoot, path) + if err != nil { + return err + } dest := filepath.Join(workDir, rel) - _ = os.MkdirAll(filepath.Dir(dest), 0o755) - data, _ := fs.ReadFile(solutionsFS, path) + if err := os.MkdirAll(filepath.Dir(dest), 0o755); err != nil { + return err + } + data, err := fs.ReadFile(solutionsFS, path) + if err != nil { + return err + } return os.WriteFile(dest, data, 0o644)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
verification.logis excluded by!**/*.log
📒 Files selected for processing (64)
.github/workflows/ci.yml(3 hunks)internal/exercises/Catalog/Concepts/01_hello.yaml(1 hunks)internal/exercises/Catalog/Concepts/02_values.yaml(1 hunks)internal/exercises/Catalog/Concepts/03_variables.yaml(1 hunks)internal/exercises/Catalog/Concepts/04_constants.yaml(1 hunks)internal/exercises/Catalog/Concepts/05_for.yaml(1 hunks)internal/exercises/Catalog/Concepts/06_if_else.yaml(1 hunks)internal/exercises/Catalog/Concepts/07_switch.yaml(1 hunks)internal/exercises/Catalog/Concepts/08_arrays.yaml(1 hunks)internal/exercises/Catalog/Concepts/09_slices.yaml(1 hunks)internal/exercises/Catalog/Concepts/10_maps.yaml(1 hunks)internal/exercises/Catalog/Concepts/11_functions.yaml(1 hunks)internal/exercises/Catalog/Concepts/12_multi_return.yaml(1 hunks)internal/exercises/Catalog/Concepts/13_variadic.yaml(1 hunks)internal/exercises/Catalog/Concepts/14_closures.yaml(1 hunks)internal/exercises/Catalog/Concepts/15_recursion.yaml(1 hunks)internal/exercises/Catalog/Concepts/16_range_built_in.yaml(1 hunks)internal/exercises/Catalog/Concepts/17_pointers.yaml(1 hunks)internal/exercises/Catalog/Concepts/18_strings_runes.yaml(1 hunks)internal/exercises/Catalog/Concepts/19_structs.yaml(1 hunks)internal/exercises/Catalog/Concepts/20_methods.yaml(1 hunks)internal/exercises/Catalog/Concepts/21_interfaces.yaml(1 hunks)internal/exercises/Catalog/Concepts/22_enums.yaml(1 hunks)internal/exercises/Catalog/Concepts/23_struct_embedding.yaml(1 hunks)internal/exercises/Catalog/Concepts/24_generics.yaml(1 hunks)internal/exercises/Catalog/Concepts/25_range_iterators.yaml(1 hunks)internal/exercises/Catalog/Concepts/26_errors.yaml(1 hunks)internal/exercises/Catalog/Concepts/27_custom_errors.yaml(1 hunks)internal/exercises/Catalog/Concepts/28_defer.yaml(1 hunks)internal/exercises/Catalog/Concepts/29_go_routines.yaml(1 hunks)internal/exercises/Catalog/Concepts/30_channels.yaml(1 hunks)internal/exercises/Catalog/Concepts/31_mutexes.yaml(1 hunks)internal/exercises/Catalog/Concepts/32_sorting.yaml(1 hunks)internal/exercises/Catalog/Concepts/33_string_formatting.yaml(1 hunks)internal/exercises/Catalog/Concepts/34_channel_buffering.yaml(1 hunks)internal/exercises/Catalog/Concepts/35_channel_sync.yaml(1 hunks)internal/exercises/Catalog/Concepts/36_json.yaml(1 hunks)internal/exercises/Catalog/Concepts/37_sorting_by_functions.yaml(1 hunks)internal/exercises/Catalog/Concepts/37_xml.yaml(1 hunks)internal/exercises/Catalog/Concepts/38_time_formatting.yaml(1 hunks)internal/exercises/Catalog/Concepts/39_channel_directions.yaml(1 hunks)internal/exercises/Catalog/Concepts/39_panic.yaml(1 hunks)internal/exercises/Catalog/Concepts/40_channel_select.yaml(1 hunks)internal/exercises/Catalog/Concepts/41_time_delay.yaml(1 hunks)internal/exercises/Catalog/Concepts/42_wait_group.yaml(1 hunks)internal/exercises/Catalog/Concepts/43_worker_pools.yaml(1 hunks)internal/exercises/Catalog/Concepts/44_atomic_counters.yaml(1 hunks)internal/exercises/Catalog/Concepts/45_range_over_channels.yaml(1 hunks)internal/exercises/Catalog/Concepts/46_non_blocking_channel_operations.yaml(1 hunks)internal/exercises/Catalog/Concepts/64_timers.yaml(1 hunks)internal/exercises/Catalog/Concepts/68_rate_limiting.yaml(1 hunks)internal/exercises/Catalog/Projects/01_text_analyzer.yaml(1 hunks)internal/exercises/Catalog/Projects/02_shape_calculator.yaml(1 hunks)internal/exercises/Catalog/Projects/03_task_scheduler.yaml(1 hunks)internal/exercises/Catalog/Projects/04_http_server.yaml(1 hunks)internal/exercises/Catalog/Projects/05_cli_todo_list.yaml(1 hunks)internal/exercises/Catalog/Projects/06_simple_chat_app.yaml(1 hunks)internal/exercises/Catalog/Projects/07_image_processing_utility.yaml(1 hunks)internal/exercises/Catalog/Projects/08_basic_key_value_store.yaml(1 hunks)internal/exercises/Catalog/Projects/09_epoch.yaml(1 hunks)internal/exercises/exercises.go(5 hunks)internal/exercises/exercises_test.go(1 hunks)internal/exercises/export_test.go(1 hunks)internal/exercises/solutions.go(5 hunks)
✅ Files skipped from review due to trivial changes (2)
- internal/exercises/Catalog/Concepts/19_structs.yaml
- internal/exercises/Catalog/Concepts/68_rate_limiting.yaml
🚧 Files skipped from review as they are similar to previous changes (52)
- internal/exercises/Catalog/Concepts/06_if_else.yaml
- internal/exercises/Catalog/Concepts/43_worker_pools.yaml
- internal/exercises/Catalog/Concepts/28_defer.yaml
- internal/exercises/Catalog/Concepts/05_for.yaml
- internal/exercises/Catalog/Projects/07_image_processing_utility.yaml
- internal/exercises/Catalog/Concepts/12_multi_return.yaml
- internal/exercises/Catalog/Concepts/46_non_blocking_channel_operations.yaml
- internal/exercises/Catalog/Concepts/04_constants.yaml
- internal/exercises/Catalog/Concepts/31_mutexes.yaml
- internal/exercises/Catalog/Concepts/18_strings_runes.yaml
- internal/exercises/Catalog/Concepts/17_pointers.yaml
- internal/exercises/Catalog/Concepts/21_interfaces.yaml
- internal/exercises/Catalog/Concepts/14_closures.yaml
- internal/exercises/Catalog/Concepts/11_functions.yaml
- internal/exercises/Catalog/Concepts/13_variadic.yaml
- internal/exercises/Catalog/Concepts/10_maps.yaml
- internal/exercises/Catalog/Concepts/42_wait_group.yaml
- internal/exercises/Catalog/Projects/05_cli_todo_list.yaml
- internal/exercises/Catalog/Concepts/39_panic.yaml
- internal/exercises/Catalog/Concepts/37_sorting_by_functions.yaml
- internal/exercises/Catalog/Concepts/33_string_formatting.yaml
- internal/exercises/Catalog/Concepts/09_slices.yaml
- internal/exercises/Catalog/Concepts/27_custom_errors.yaml
- internal/exercises/Catalog/Concepts/45_range_over_channels.yaml
- internal/exercises/Catalog/Projects/02_shape_calculator.yaml
- internal/exercises/Catalog/Concepts/07_switch.yaml
- internal/exercises/exercises_test.go
- internal/exercises/Catalog/Concepts/36_json.yaml
- internal/exercises/Catalog/Concepts/15_recursion.yaml
- .github/workflows/ci.yml
- internal/exercises/Catalog/Concepts/26_errors.yaml
- internal/exercises/Catalog/Concepts/44_atomic_counters.yaml
- internal/exercises/Catalog/Concepts/20_methods.yaml
- internal/exercises/Catalog/Projects/06_simple_chat_app.yaml
- internal/exercises/Catalog/Projects/08_basic_key_value_store.yaml
- internal/exercises/Catalog/Concepts/37_xml.yaml
- internal/exercises/Catalog/Concepts/35_channel_sync.yaml
- internal/exercises/Catalog/Concepts/30_channels.yaml
- internal/exercises/Catalog/Concepts/64_timers.yaml
- internal/exercises/Catalog/Concepts/38_time_formatting.yaml
- internal/exercises/Catalog/Concepts/08_arrays.yaml
- internal/exercises/Catalog/Concepts/34_channel_buffering.yaml
- internal/exercises/Catalog/Concepts/01_hello.yaml
- internal/exercises/Catalog/Projects/09_epoch.yaml
- internal/exercises/Catalog/Concepts/40_channel_select.yaml
- internal/exercises/Catalog/Concepts/16_range_built_in.yaml
- internal/exercises/Catalog/Concepts/39_channel_directions.yaml
- internal/exercises/Catalog/Concepts/22_enums.yaml
- internal/exercises/Catalog/Concepts/29_go_routines.yaml
- internal/exercises/Catalog/Concepts/02_values.yaml
- internal/exercises/Catalog/Projects/04_http_server.yaml
- internal/exercises/Catalog/Concepts/32_sorting.yaml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bdeb1337
Repo: zhravan/golearn PR: 132
File: internal/exercises/solutions/06_if_else/if_else.go:1-12
Timestamp: 2025-10-04T21:41:57.043Z
Learning: In the golearn repository, exercise solutions belong in `internal/exercises/solutions/$ex/` (reference implementations), while `internal/exercises/templates/$ex/` contains starter code with TODOs and test files for learners.
📚 Learning: 2025-10-04T21:41:57.043Z
Learnt from: bdeb1337
Repo: zhravan/golearn PR: 132
File: internal/exercises/solutions/06_if_else/if_else.go:1-12
Timestamp: 2025-10-04T21:41:57.043Z
Learning: In the golearn repository, exercise solutions belong in `internal/exercises/solutions/$ex/` (reference implementations), while `internal/exercises/templates/$ex/` contains starter code with TODOs and test files for learners.
Applied to files:
internal/exercises/solutions.gointernal/exercises/exercises.go
🧬 Code graph analysis (2)
internal/exercises/solutions.go (1)
internal/exercises/exercises.go (1)
Get(166-187)
internal/exercises/export_test.go (1)
internal/exercises/exercises.go (1)
Catalog(40-43)
🔇 Additional comments (11)
internal/exercises/Catalog/Concepts/25_range_iterators.yaml (1)
1-5: LGTM!The exercise descriptor is well-formed with all required fields present.
internal/exercises/Catalog/Concepts/23_struct_embedding.yaml (1)
1-5: LGTM!The exercise descriptor is correctly structured.
internal/exercises/Catalog/Concepts/03_variables.yaml (1)
1-5: LGTM!The exercise definition is complete and follows the established pattern.
internal/exercises/Catalog/Concepts/41_time_delay.yaml (1)
1-8: LGTM!The exercise descriptor is well-structured with helpful hints for learners.
internal/exercises/solutions.go (1)
21-35: LGTM! Exercise metadata retrieval added.The function now correctly retrieves exercise metadata to determine the source directory, which properly supports exercises with custom directory names (via the
Dirfield).internal/exercises/export_test.go (1)
1-27: LGTM! Well-designed test helper.The helper correctly:
- Protects catalog state with a mutex
- Releases the lock before calling
testFunc()(allowing tests to callcatalog())- Restores original state via defer (even on panic)
- Forces a reload by clearing
catalogDataThis enables safe, isolated testing of catalog loading behavior.
internal/exercises/exercises.go (4)
24-38: LGTM! Path abstraction properly supports custom directories.The new
Dirfield andPath()method cleanly support exercises where the filesystem directory name differs from the slug (e.g., Projects with numeric prefixes).
45-76: LGTM! Testable catalog loading with fallback.The refactored singleton pattern:
- Uses
sync.Mutexinstead ofsync.Onceto enable test resets- Provides
catalogLoaderwrapper for dependency injection- Falls back to a minimal in-code catalog on load failures
This design properly balances production resilience with test flexibility.
78-124: LGTM! Robust directory-based catalog loading.The implementation correctly:
- Loads exercises from both
Catalog/ConceptsandCatalog/Projects- Handles missing directories gracefully (returns empty slice)
- Validates that at least one directory has content
- Sorts exercises by slug for deterministic ordering
- Validates required fields (slug presence)
- Provides clear error messages for parse failures
216-245: Note the filepath.Rel fallback on line 230.The error handling for
filepath.Relon line 228 falls back tostrings.TrimPrefixon line 230. While this handles most cases, be aware that iffilepath.Relfails for reasons other than incompatible path formats, the fallback might produce unexpected results.The current approach is acceptable for the embedded FS use case where paths are predictable, but consider logging the error if you add observability later.
internal/exercises/Catalog/Concepts/24_generics.yaml (1)
1-5: No changes needed—test_regex: ".*"is the correct and standard pattern.All exercises use this pattern consistently, and test execution is already scoped to the exercise directory via
go test -runwithin that directory. The broad regex is safe and intentional.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/exercises/solutions.go (1)
23-101: Don’t ignore copy errors; also avoidfilepath.Joinfor embedded roots.Right now
_ = fs.ReadFile(...)can silently produce empty output, andfilepath.Join("templates", srcDir)/filepath.Join("solutions", srcDir)can be wrong on Windows forWalkDir.func CreateSolutionSandbox(slug string) (string, func(), error) { // Look up exercise to get the correct source Directory ex, err := Get(slug) if err != nil { return "", func() {}, err } srcDir := ex.Path() // Uses Dir if set, else Slug if !templateExists(srcDir) { return "", func() {}, errors.New("no template found for exercise") } if !SolutionExists(srcDir) { return "", func() {}, errors.New("no embedded solution available") } ... - // Copy tests from templates (using srcDir 101) - templateRoot := filepath.Join("templates", srcDir) - if err := fs.WalkDir(templatesFS, templateRoot, func(path string, d fs.DirEntry, err error) error { + // Copy tests from templates (using srcDir 101) + templateRoot := pathpkg.Join("templates", srcDir) + if err := fs.WalkDir(templatesFS, templateRoot, func(fpath string, d fs.DirEntry, err error) error { if err != nil { return err } if d.IsDir() { return nil } - if !strings.HasSuffix(path, "_test.go") { + if !strings.HasSuffix(fpath, "_test.go") { return nil } - - rel, _ := filepath.Rel(templateRoot, path) + rel, err := pathpkg.Rel(templateRoot, fpath) + if err != nil { + return err + } dest := filepath.Join(workDir, rel) - _ = os.MkdirAll(filepath.Dir(dest), 0o755) - data, _ := fs.ReadFile(templatesFS, path) + if err := os.MkdirAll(filepath.Dir(dest), 0o755); err != nil { + return err + } + data, err := fs.ReadFile(templatesFS, fpath) + if err != nil { + return err + } return os.WriteFile(dest, data, 0o644) }); err != nil { cleanup() return "", func() {}, err } // Copy solution implementation from solutions (using srcDir 101) - solutionRoot := filepath.Join("solutions", srcDir) - if err := fs.WalkDir(solutionsFS, solutionRoot, func(path string, d fs.DirEntry, err error) error { + solutionRoot := pathpkg.Join("solutions", srcDir) + if err := fs.WalkDir(solutionsFS, solutionRoot, func(fpath string, d fs.DirEntry, err error) error { if err != nil { return err } if d.IsDir() { return nil } - if strings.HasSuffix(path, "_test.go") { + if strings.HasSuffix(fpath, "_test.go") { return nil } - - rel, _ := filepath.Rel(solutionRoot, path) + rel, err := pathpkg.Rel(solutionRoot, fpath) + if err != nil { + return err + } dest := filepath.Join(workDir, rel) - _ = os.MkdirAll(filepath.Dir(dest), 0o755) - data, _ := fs.ReadFile(solutionsFS, path) + if err := os.MkdirAll(filepath.Dir(dest), 0o755); err != nil { + return err + } + data, err := fs.ReadFile(solutionsFS, fpath) + if err != nil { + return err + } return os.WriteFile(dest, data, 0o644) }); err != nil { cleanup() return "", func() {}, err }
🧹 Nitpick comments (3)
internal/exercises/exercises.go (3)
75-121: Consider rejecting duplicate slugs while loading the catalog.Right now duplicates will both load and later lookups become ambiguous.
func loadExercisesDir(fsys fs.FS, dir string) ([]Exercise, error) { var exercises []Exercise + seen := map[string]struct{}{} entries, err := fs.ReadDir(fsys, dir) ... if ex.Slug == "" { return nil, fmt.Errorf("exercise in %s missing slug", e.Name()) } + if _, ok := seen[ex.Slug]; ok { + return nil, fmt.Errorf("duplicate exercise slug %q in %s", ex.Slug, dir) + } + seen[ex.Slug] = struct{}{} exercises = append(exercises, ex) }
220-250: Usepath.Relfor embed-FS paths (avoid mixed separator logic).
fpathfromfs.WalkDiroverembed.FSis slash-separated, sopath.Rel(root, fpath)is the natural fit; then convert to OS paths when joining totargetDir.root := path.Join("templates", ex.Path()) return fs.WalkDir(templatesFS, root, func(fpath string, d fs.DirEntry, err error) error { ... - rel, err := filepath.Rel(root, filepath.FromSlash(fpath)) + rel, err := path.Rel(root, fpath) if err != nil { - rel = strings.TrimPrefix(fpath, root+"/") + return err } if rel == "." { return nil } - dest := filepath.Join(targetDir, rel) + dest := filepath.Join(targetDir, filepath.FromSlash(rel))
54-73: Optional: add a breadcrumb when falling back tofallbackCatalog().Today any embedded catalog parse/read error becomes indistinguishable from “no exercises shipped” because fallback is silent; even a debug log (if you have a logger) would make support easier.
Also applies to: 123-133
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.github/workflows/ci.yml(3 hunks)internal/exercises/Catalog/Projects/01_text_analyzer.yaml(1 hunks)internal/exercises/Catalog/Projects/02_shape_calculator.yaml(1 hunks)internal/exercises/Catalog/Projects/03_task_scheduler.yaml(1 hunks)internal/exercises/Catalog/Projects/04_http_server.yaml(1 hunks)internal/exercises/Catalog/Projects/05_cli_todo_list.yaml(1 hunks)internal/exercises/Catalog/Projects/06_simple_chat_app.yaml(1 hunks)internal/exercises/Catalog/Projects/07_image_processing_utility.yaml(1 hunks)internal/exercises/Catalog/Projects/08_basic_key_value_store.yaml(1 hunks)internal/exercises/Catalog/Projects/09_epoch.yaml(1 hunks)internal/exercises/exercises.go(5 hunks)internal/exercises/solutions.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- .github/workflows/ci.yml
- internal/exercises/Catalog/Projects/07_image_processing_utility.yaml
- internal/exercises/Catalog/Projects/09_epoch.yaml
- internal/exercises/Catalog/Projects/01_text_analyzer.yaml
- internal/exercises/Catalog/Projects/03_task_scheduler.yaml
- internal/exercises/Catalog/Projects/02_shape_calculator.yaml
- internal/exercises/Catalog/Projects/05_cli_todo_list.yaml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bdeb1337
Repo: zhravan/golearn PR: 132
File: internal/exercises/solutions/06_if_else/if_else.go:1-12
Timestamp: 2025-10-04T21:41:57.043Z
Learning: In the golearn repository, exercise solutions belong in `internal/exercises/solutions/$ex/` (reference implementations), while `internal/exercises/templates/$ex/` contains starter code with TODOs and test files for learners.
📚 Learning: 2025-10-04T21:41:57.043Z
Learnt from: bdeb1337
Repo: zhravan/golearn PR: 132
File: internal/exercises/solutions/06_if_else/if_else.go:1-12
Timestamp: 2025-10-04T21:41:57.043Z
Learning: In the golearn repository, exercise solutions belong in `internal/exercises/solutions/$ex/` (reference implementations), while `internal/exercises/templates/$ex/` contains starter code with TODOs and test files for learners.
Applied to files:
internal/exercises/exercises.gointernal/exercises/solutions.go
🧬 Code graph analysis (1)
internal/exercises/solutions.go (1)
internal/exercises/exercises.go (1)
Get(167-188)
🔇 Additional comments (4)
internal/exercises/Catalog/Projects/04_http_server.yaml (3)
4-4: Verify test_regex pattern is intentional.The test pattern
".*"is very broad and matches everything. Confirm this is the intended behavior for test discovery in this exercise, or consider a more specific pattern if appropriate.
1-6: The exercise directory structure is correctly configured. Bothinternal/exercises/templates/104_http_server/andinternal/exercises/solutions/104_http_server/exist with appropriate files (starter code template and test file, plus reference implementation).
1-2: No action needed. Thesluganddirvalues follow an intentional design pattern: the slug (04_http_server) serves as the user-facing exercise identifier, while thedir(104_http_server) references the internal directory structure with a category prefix. Both thesolutions/104_http_serverandtemplates/104_http_serverdirectories exist and are properly configured.internal/exercises/Catalog/Projects/06_simple_chat_app.yaml (1)
1-6: Catalog entry looks consistent (slug vs dir separation is clear).
slugis the external identifier, whiledircleanly maps to the templates/solutions folder name.
internal/exercises/Catalog/Projects/08_basic_key_value_store.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/exercises/exercises.go (1)
220-250: Fix relative-path derivation incopyExerciseTemplate(mixingpathvsfilepathis brittle).
templatesFSpaths (fpath) are always slash-separated. Usingfilepath.Rel(root, filepath.FromSlash(fpath))mixes OS separators with slash paths and can break on Windows / non-standard separators. Preferpath.Relfor embedded paths, then convert once for the destination.func copyExerciseTemplate(ex Exercise) error { @@ root := path.Join("templates", ex.Path()) return fs.WalkDir(templatesFS, root, func(fpath string, d fs.DirEntry, err error) error { @@ - rel, err := filepath.Rel(root, filepath.FromSlash(fpath)) - if err != nil { - rel = strings.TrimPrefix(fpath, root+"/") - } + rel, err := path.Rel(root, fpath) + if err != nil { + return err + } if rel == "." { return nil } - dest := filepath.Join(targetDir, rel) + dest := filepath.Join(targetDir, filepath.FromSlash(rel)) if d.IsDir() { return os.MkdirAll(dest, 0o755) } data, err := fs.ReadFile(templatesFS, fpath)
🧹 Nitpick comments (2)
internal/exercises/exercises.go (2)
24-38: ValidateExercise.Dir/Exercise.Slugto prevent../ separators (future-hardening).
ex.Path()feeds intopath.Join("templates", ex.Path()). Even if today the catalog is embedded (trusted), adding basic validation keeps invariants tight and avoids accidental escapes if loader inputs ever become user-controlled.func loadExercisesDir(fsys fs.FS, dir string) ([]Exercise, error) { @@ var ex Exercise if err := yaml.Unmarshal(data, &ex); err != nil { return nil, fmt.Errorf("parse %s: %w", e.Name(), err) } if ex.Slug == "" { return nil, fmt.Errorf("exercise in %s missing slug", e.Name()) } + // Hardening: keep identifiers as simple directory names. + for fieldName, v := range map[string]string{"slug": ex.Slug, "dir": ex.Dir} { + if v == "" { + continue + } + if strings.Contains(v, "/") || strings.Contains(v, `\`) || strings.Contains(v, "..") || path.Clean(v) != v { + return nil, fmt.Errorf("%s in %s is not a safe directory name: %q", fieldName, e.Name(), v) + } + } exercises = append(exercises, ex) }
75-121: Detect duplicateSlug/Dircollisions during load to avoid ambiguousGet().
Right now duplicates across Concepts/Projects (orSlugvs someone else’sDir) will “work” butGet()will return the first match (Concepts wins), which is brittle.func loadCatalogFromFS(fsys fs.FS) (Catalog, error) { @@ if len(concepts) == 0 && len(projects) == 0 { return Catalog{}, errors.New("no exercises found in Catalog/") } + seen := map[string]string{} // key -> origin + check := func(origin string, ex Exercise) error { + for _, k := range []string{ex.Slug, ex.Dir} { + if k == "" { + continue + } + if prev, ok := seen[k]; ok { + return fmt.Errorf("duplicate exercise identifier %q: %s vs %s", k, prev, origin) + } + seen[k] = origin + } + return nil + } + for _, ex := range concepts { + if err := check("Catalog/Concepts/"+ex.Slug, ex); err != nil { return Catalog{}, err } + } + for _, ex := range projects { + if err := check("Catalog/Projects/"+ex.Slug, ex); err != nil { return Catalog{}, err } + } return Catalog{Concepts: concepts, Projects: projects}, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exercises/exercises.go(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bdeb1337
Repo: zhravan/golearn PR: 132
File: internal/exercises/solutions/06_if_else/if_else.go:1-12
Timestamp: 2025-10-04T21:41:57.043Z
Learning: In the golearn repository, exercise solutions belong in `internal/exercises/solutions/$ex/` (reference implementations), while `internal/exercises/templates/$ex/` contains starter code with TODOs and test files for learners.
📚 Learning: 2025-10-04T21:41:57.043Z
Learnt from: bdeb1337
Repo: zhravan/golearn PR: 132
File: internal/exercises/solutions/06_if_else/if_else.go:1-12
Timestamp: 2025-10-04T21:41:57.043Z
Learning: In the golearn repository, exercise solutions belong in `internal/exercises/solutions/$ex/` (reference implementations), while `internal/exercises/templates/$ex/` contains starter code with TODOs and test files for learners.
Applied to files:
internal/exercises/exercises.go
🔇 Additional comments (3)
internal/exercises/exercises.go (3)
21-23: EmbeddingCatalog/**looks correct, but keep case/paths consistent across platforms.
Go embed paths are slash-based and case-sensitive at build time on many CI environments; ensure the on-disk directory is exactlyinternal/exercises/Catalog/...and not a casing variant.
166-188:Get()matchingSlugorDiris good; consider clarifying precedence in docs/tests.
If a user passes a value that matches both a Concept and Project identifier, Concepts will win due to search order—fine, but worth being explicit (or prevented via duplicate detection).
45-73: WithTestCatalogLoader already handles safe test isolation correctly.The exported
WithTestCatalogLoaderhelper ininternal/exercises/export_test.goproperly addresses the concern: it lockscatalogMu, resetscatalogData = nil, swapscatalogLoader, then restores both under the mutex in a defer block. Tests using this helper (e.g.,TestCatalogOverride) are already safe from ordering-dependent failures.
- Fixed CI workflows: resolved cache permission errors and added robust solution verification checks. - Added tests for the new catalog loader using in-memory filesystems.
LeeFred3042U
force-pushed
the
feature/catalog-directory-loader
branch
from
17ad3da to
ee00e3e
Compare
|
@kaushalyap suggestion/? on what I have to do here |
1 participant
Summary
This PR replaces the previous single-file catalog.yaml with a directory-based catalog stored under embedded Catalog/** folders. The one-file approach caused frequent merge conflicts because every exercise lived in the same YAML document. Splitting the catalog so each exercise has its own YAML file eliminates those conflicts and makes adding new exercises much simpler.
catalog() now loads from Catalog/Concepts and Catalog/Projects using a new loadCatalogFromFS helper. Each exercise is defined by its own YAML file (e.g. 01_hello.yaml). If loading fails—or both directories are empty—the system falls back to the existing minimal inline catalog (01_hello) to preserve current behavior.
Tests using fstest.MapFS cover directory loading, ordering by slug, invalid YAML handling, ignored non-YAML files, missing directories, and fallback logic.
No CLI behavior changes. Existing public functions (ListAll, Get, InitAll, Reset) continue to work the same with the new catalog structure.
Checklist
internal/exercisestests pass (catalog loader, FS logic, fallback behavior).Exercise verification (
make verifyorgolearn verify) is expected to fail on a clean repo because exercises contain TODOs.fstestused for tests)Screenshots / Output (if CLI UX)
Not applicable — feature is internal.
Behavior validated through automated tests:
Related issues
Fixes #113
Summary by CodeRabbit
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.