120_time - added time exercise #114
Conversation
WalkthroughAdds a new beginner "Time" exercise (slug Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Learner
participant Docs as "docs/exercises.md"
participant Catalog as "internal/exercises/catalog.yaml"
participant Template as "internal/exercises/templates/120_time/time.go"
participant Tests as "internal/exercises/templates/120_time/time_test.go"
participant Solution as "internal/exercises/solutions/120_time/time.go"
Learner->>Docs: Browse exercises list
Docs-->>Learner: Show "120_time - Time"
Learner->>Catalog: Request exercise details (slug 120_time)
Catalog-->>Learner: Provide template/test paths and hints
Learner->>Template: Open/implement template code
Learner->>Tests: Run tests
Tests->>Template: Call FormatDate/FormatDateTime/Parse*/DaysBetween/AddDays
Template-->>Tests: Return results
Tests-->>Learner: Report pass/fail
Note right of Solution: Reference solution available
Solution-->>Learner: Provide example implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (5)
internal/exercises/templates/120_time/time_test.go (4)
13-21: LGTM!The test correctly verifies the formatting behavior. For enhanced coverage, consider adding edge cases like single-digit months and days (e.g.,
2025-01-05).
23-31: LGTM!The test correctly verifies the formatting behavior. For enhanced coverage, consider testing non-midnight times (e.g.,
14:30:45).
62-70: LGTM!The test correctly verifies the parsing behavior. For enhanced coverage, consider testing invalid inputs to ensure zero time is returned on error.
72-80: LGTM!The test correctly verifies the parsing behavior. For enhanced coverage, consider testing invalid inputs and non-midnight times.
internal/exercises/templates/120_time/time.go (1)
1-45: Template matches solution with DST limitation.This template file is identical to the solution, which is appropriate for a reference implementation. The
DaysBetweenfunction has the same DST limitation noted in the solution file (line 15-19 of internal/exercises/solutions/120_time/time.go).For consistency, consider adding the same educational comment about DST limitations here as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/exercises.md(1 hunks)internal/exercises/catalog.yaml(1 hunks)internal/exercises/solutions/120_time/time.go(1 hunks)internal/exercises/templates/120_time/time.go(1 hunks)internal/exercises/templates/120_time/time_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/exercises/templates/120_time/time_test.go (1)
internal/exercises/templates/120_time/time.go (6)
FormatDate(6-8)FormatDateTime(11-13)DaysBetween(17-19)AddDays(22-24)ParseDate(28-34)ParseDateTime(38-44)
internal/exercises/templates/120_time/time.go (1)
internal/exercises/solutions/120_time/time.go (6)
FormatDate(6-8)FormatDateTime(11-13)DaysBetween(17-19)AddDays(22-24)ParseDate(28-34)ParseDateTime(38-44)
🔇 Additional comments (9)
internal/exercises/solutions/120_time/time.go (5)
5-8: LGTM!The function correctly formats dates using Go's reference layout.
10-13: LGTM!The function correctly formats date-times using Go's reference layout.
21-24: LGTM!The function correctly uses
AddDateto add days, which handles month/year rollovers and DST transitions properly.
26-34: LGTM!The function correctly parses date strings. Returning zero time on error is acceptable for a beginner exercise (callers can check with
IsZero()).
36-44: LGTM!The function correctly parses date-time strings, consistent with
ParseDate.docs/exercises.md (1)
71-71: LGTM!The documentation entry is correctly formatted and consistent with the catalog.
internal/exercises/templates/120_time/time_test.go (3)
8-11: LGTM!The helper function correctly creates UTC dates, avoiding DST complications in tests.
33-50: LGTM!The test correctly verifies both positive and negative differences.
52-60: LGTM!The test correctly verifies the date addition using
time.Time.Equal.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/exercises/solutions/120_time/time.go (2)
32-40: Consider returning an error for better Go practices.The function returns a zero time on parse failure, which simplifies the API but hides errors. While acceptable for a beginner exercise, idiomatic Go prefers explicit error returns:
func ParseDate(s string) (time.Time, error) { return time.Parse("2006-01-02", s) }However, if simplicity is the pedagogical goal, the current implementation is fine. Callers can check
IsZero()to detect failures.
42-50: Consider returning an error for better Go practices.Similar to
ParseDate, returning a zero time on parse failure simplifies the API but hides errors. For teaching purposes, consider whether explicit error returns would better demonstrate idiomatic Go:func ParseDateTime(s string) (time.Time, error) { return time.Parse("2006-01-02 15:04:05", s) }However, if simplicity is the pedagogical goal, the current implementation is acceptable. Callers can check
IsZero()to detect failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/exercises/catalog.yaml(1 hunks)internal/exercises/solutions/120_time/time.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exercises/catalog.yaml
🔇 Additional comments (4)
internal/exercises/solutions/120_time/time.go (4)
5-8: LGTM!The function correctly formats dates using Go's reference time layout. The implementation is idiomatic and appropriate for a beginner exercise.
10-13: LGTM!The function correctly formats date-time values using Go's reference time layout. The implementation is idiomatic and appropriate for a beginner exercise.
15-24: LGTM! DST limitation documented.The function now includes a clear comment documenting the DST edge case, which addresses the previous review feedback. For a beginner exercise, this implementation with documentation is appropriate.
27-30: LGTM!The function correctly uses
AddDateto add days, which properly handles month and year boundaries. The implementation is idiomatic and appropriate for a beginner exercise.
|
@zhravan plz accept this PR |
|
@Vaibhavsg17 : Can you resolve the conflicts please? |
|
@zhravan conflict solved |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
internal/exercises/templates/120_time/time_test.go (6)
15-23: Consider adding edge case coverage.The test correctly validates the happy path for
FormatDate. For a more robust test suite, consider adding cases for:
- Single-digit months (January)
- Month/year boundaries (e.g., December 31st)
- Leap year dates (February 29th)
However, for a beginner exercise, the current coverage may be intentionally minimal.
25-33: Add test coverage for non-midnight times.The test only validates formatting at midnight (00:00:00) since
mustDatecreates dates at midnight. Consider adding a test case with actual time components to ensure the time formatting works correctly:+func TestFormatDateTime_WithTime(t *testing.T) { + d := time.Date(2025, time.October, 2, 14, 30, 45, 0, time.UTC) + want := "2025-10-02 14:30:45" + got := timeutils.FormatDateTime(d) + if got != want { + t.Fatalf("FormatDateTime(%v) = %q; want %q", d, got, want) + } +}
35-52: LGTM! Consider adding boundary test cases.The test correctly validates both positive and negative day differences. For more comprehensive coverage, consider adding:
- Same day test (0 days difference)
- Month boundary crossing
- Year boundary crossing
Note: The implementation uses
Hours()/24with integer truncation, which works for whole-day differences but could be fragile if the underlying implementation changes or times aren't normalized.
54-62: LGTM! Consider testing negative day additions.The test correctly validates adding positive days. For completeness, consider adding a test case for negative days (subtraction):
+func TestAddDays_Negative(t *testing.T) { + d := mustDate(2025, time.October, 11) + got := timeutils.AddDays(d, -10) + want := mustDate(2025, time.October, 1) + if !got.Equal(want) { + t.Fatalf("AddDays(%v, -10) = %v; want %v", d, got, want) + } +}
64-72: Add test coverage for invalid date input.The test only validates the happy path. Since
ParseDatecan fail (returningtime.Time{}on error per the implementation), add a test case for invalid input:+func TestParseDate_Invalid(t *testing.T) { + tests := []string{ + "invalid", + "2025-13-01", // invalid month + "2025-10-32", // invalid day + "10-02-2025", // wrong format + } + for _, input := range tests { + got := timeutils.ParseDate(input) + if !got.IsZero() { + t.Errorf("ParseDate(%q) should return zero time for invalid input, got %v", input, got) + } + } +}
74-82: Add test coverage for invalid input and non-midnight times.Similar to
TestParseDate, this test only validates the happy path at midnight. Add coverage for:
- Invalid input handling:
+func TestParseDateTime_Invalid(t *testing.T) { + tests := []string{ + "invalid", + "2025-10-02", // missing time + "2025-10-02 25:00:00", // invalid hour + } + for _, input := range tests { + got := timeutils.ParseDateTime(input) + if !got.IsZero() { + t.Errorf("ParseDateTime(%q) should return zero time for invalid input, got %v", input, got) + } + } +}
- Non-midnight times:
+func TestParseDateTime_WithTime(t *testing.T) { + input := "2025-10-02 14:30:45" + want := time.Date(2025, time.October, 2, 14, 30, 45, 0, time.UTC) + got := timeutils.ParseDateTime(input) + if !got.Equal(want) { + t.Fatalf("ParseDateTime(%q) = %v; want %v", input, got, want) + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/exercises.md(1 hunks)internal/exercises/catalog.yaml(1 hunks)internal/exercises/solutions/120_time/time.go(1 hunks)internal/exercises/templates/120_time/time.go(1 hunks)internal/exercises/templates/120_time/time_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/exercises.md
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/exercises/solutions/120_time/time.go
- internal/exercises/catalog.yaml
- internal/exercises/templates/120_time/time.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/exercises/templates/120_time/time_test.go (1)
internal/exercises/templates/120_time/time.go (6)
FormatDate(6-8)FormatDateTime(11-13)DaysBetween(17-19)AddDays(22-24)ParseDate(28-34)ParseDateTime(38-44)
🔇 Additional comments (1)
internal/exercises/templates/120_time/time_test.go (1)
10-13: LGTM!The
mustDatehelper cleanly creates UTC dates at midnight, which is appropriate for testing date operations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
internal/exercises/templates/120_time/time_test.go (5)
16-23: Consider adding edge cases for more thorough coverage.The test validates the basic formatting, but consider adding test cases for:
- Single-digit months and days (e.g., "2025-01-05")
- Leap year dates (e.g., "2024-02-29")
- Year boundaries (e.g., "2025-12-31")
Example expansion:
func TestFormatDate(t *testing.T) { - d := mustDate(2025, time.October, 2) - want := "2025-10-02" - got := timeutils.FormatDate(d) - if got != want { - t.Fatalf("FormatDate(%v) = %q; want %q", d, got, want) - } + tests := []struct { + name string + date time.Time + want string + }{ + {"October date", mustDate(2025, time.October, 2), "2025-10-02"}, + {"Single digit month", mustDate(2025, time.January, 5), "2025-01-05"}, + {"Year end", mustDate(2025, time.December, 31), "2025-12-31"}, + {"Leap year", mustDate(2024, time.February, 29), "2024-02-29"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := timeutils.FormatDate(tt.date) + if got != tt.want { + t.Fatalf("FormatDate(%v) = %q; want %q", tt.date, got, tt.want) + } + }) + } }
26-33: Consider testing non-midnight times.The test only validates midnight (00:00:00). For more comprehensive coverage, consider adding test cases with non-zero hours, minutes, and seconds.
Example:
func TestFormatDateTime(t *testing.T) { d := mustDate(2025, time.October, 2) want := "2025-10-02 00:00:00" got := timeutils.FormatDateTime(d) if got != want { t.Fatalf("FormatDateTime(%v) = %q; want %q", d, got, want) } + + // Test with non-zero time + d2 := time.Date(2025, time.October, 2, 15, 30, 45, 0, time.UTC) + want2 := "2025-10-02 15:30:45" + got2 := timeutils.FormatDateTime(d2) + if got2 != want2 { + t.Fatalf("FormatDateTime(%v) = %q; want %q", d2, got2, want2) + } }
36-52: Add edge cases and verify DST handling.The current tests validate positive and negative differences, but consider adding:
- Same date (0 days):
DaysBetween(d, d)should return0- Dates with non-zero times to verify truncation behavior
- Dates spanning month/year boundaries
Additionally, the implementation uses
Hours() / 24, which may produce unexpected results during DST transitions or with non-midnight times. While this is acceptable for a beginner exercise using UTC dates at midnight, it's worth documenting this limitation.Add test cases:
// Same date got = timeutils.DaysBetween(start, start) want = 0 if got != want { t.Fatalf("DaysBetween(%v, %v) = %d; want %d", start, start, got, want) } // Non-midnight times (23 hours apart = 0 days due to truncation) d1 := time.Date(2025, time.October, 1, 0, 0, 0, 0, time.UTC) d2 := time.Date(2025, time.October, 1, 23, 0, 0, 0, time.UTC) got = timeutils.DaysBetween(d1, d2) want = 0 // 23 hours / 24 = 0 (truncated) if got != want { t.Fatalf("DaysBetween(%v, %v) = %d; want %d", d1, d2, got, want) }
55-62: Consider testing negative days and boundary cases.The test validates adding positive days, but consider adding:
- Negative days:
AddDays(d, -5)- Zero days:
AddDays(d, 0)- Month boundaries: October 31 + 1 day = November 1
- Year boundaries: December 31 + 1 day = January 1
Example additions:
// Negative days gotNeg := timeutils.AddDays(d, -5) wantNeg := mustDate(2025, time.September, 26) if !gotNeg.Equal(wantNeg) { t.Fatalf("AddDays(%v, -5) = %v; want %v", d, gotNeg, wantNeg) } // Month boundary dEnd := mustDate(2025, time.October, 31) gotMonth := timeutils.AddDays(dEnd, 1) wantMonth := mustDate(2025, time.November, 1) if !gotMonth.Equal(wantMonth) { t.Fatalf("AddDays(%v, 1) = %v; want %v", dEnd, gotMonth, wantMonth) }
65-82: Add tests for invalid input to verify error handling.Both
TestParseDateandTestParseDateTimeonly test valid inputs. The implementations return a zerotime.Timeon parse errors, which means:
- Invalid inputs like
"invalid","2025-13-45", or""will returntime.Time{}(January 1, year 1, 00:00:00 UTC)- There's no way for callers to distinguish between parse errors and intentional zero times
Consider adding tests for error cases to document this behavior:
func TestParseDate_Invalid(t *testing.T) { tests := []struct { name string input string }{ {"empty string", ""}, {"invalid format", "2025/10/02"}, {"invalid date", "2025-13-45"}, {"partial date", "2025-10"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got := timeutils.ParseDate(tt.input) if !got.IsZero() { t.Fatalf("ParseDate(%q) = %v; want zero time for invalid input", tt.input, got) } }) } } func TestParseDateTime_Invalid(t *testing.T) { tests := []struct { name string input string }{ {"empty string", ""}, {"invalid format", "2025/10/02 00:00:00"}, {"missing time", "2025-10-02"}, {"invalid time", "2025-10-02 25:00:00"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got := timeutils.ParseDateTime(tt.input) if !got.IsZero() { t.Fatalf("ParseDateTime(%q) = %v; want zero time for invalid input", tt.input, got) } }) } }Note: The current error handling approach (returning zero time instead of an error) is atypical for Go. Consider whether the exercise should teach proper error handling by having these functions return
(time.Time, error)instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exercises/templates/120_time/time_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/exercises/templates/120_time/time_test.go (1)
internal/exercises/templates/120_time/time.go (6)
FormatDate(6-8)FormatDateTime(11-13)DaysBetween(17-19)AddDays(22-24)ParseDate(28-34)ParseDateTime(38-44)
🔇 Additional comments (2)
internal/exercises/templates/120_time/time_test.go (2)
10-13: LGTM!The helper function correctly creates UTC dates at midnight, which is appropriate for testing date formatting and manipulation functions.
7-7: Import path and package declaration align correctly. Verifiedtime.godeclarespackage timeutilsand tests importgithub.com/zhravan/golearn/internal/exercises/templates/120_timeas expected; no changes needed.
|
@zhravan i guess the pr is ready to get merged |
|
@zhravan could you please merge the PR |
|
@zhravan this pr is ready to get merged |
2 participants
Summary
Added the 120_time exercise to teach learners about formatting, parsing, and calculating dates in Go.
This includes the template for learners, the test file, and a reference solution.
Describe the change and its motivation.
Added a new exercise for Time functions (120_time), including:
Checklist
make verifyorgolearn verify <slug>Screenshots / Output (if CLI UX)
Paste before/after where helpful.
Related issues
Fixes #83
Summary by CodeRabbit
New Features
Documentation
Tests