added epoch - #89
Conversation
WalkthroughAdds an "Epoch Conversion" exercise: a catalog entry, template with two placeholder conversion functions, unit tests for bidirectional conversion, and a reference solution implementing epoch↔formatted UTC time conversions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer/Test
participant T as Tests
participant E as epoch package
participant G as time package
Dev->>T: run tests
T->>E: EpochToTime(epoch)
alt solution
E->>G: time.Unix(epoch,0).UTC().Format(layout)
G-->>E: formatted string
E-->>T: formatted string
else template (placeholder)
E-->>T: ""
end
T->>E: TimeToEpoch(input)
alt solution
E->>G: time.ParseInLocation(layout,input,UTC)
G-->>E: time.Time / error
E->>E: t.Unix()
E-->>T: epoch (int64)
else template (placeholder)
E-->>T: 0
end
T-->>Dev: pass / fail
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Tip 🧪 Early access (models): enabledWe are currently testing Sonnet 4.5 code review models, which should lead to better review quality. However, this model may result in higher noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/exercises/catalog.yaml(1 hunks)internal/exercises/templates/36_epoch/epoch.go(1 hunks)internal/exercises/templates/36_epoch/epoch_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/exercises/templates/36_epoch/epoch_test.go (1)
internal/exercises/templates/36_epoch/epoch.go (2)
EpochToTime(6-9)TimeToEpoch(12-15)
|
Can u review now |
zhravan
left a comment
zhravan
left a comment
There was a problem hiding this comment.
@Omesh2004 Nice work!! ⭐
Can you simulate the solution as well in the solutions section please?
Otherwise overall LGTM
|
i have added the solution in /internal/exercises/solutions/36_epoch route |
|
One last change, instead specific time, let's take current system time? Let's have multiple format like Unix format and other formats too? |
zhravan
added
enhancement
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
internal/exercises/solutions/36_epoch/epoch.go (1)
15-17: Consider usingUnixMilli()for cleaner code.The manual calculation is correct, but Go 1.17+ provides
time.Now().UnixMilli()which is more idiomatic.func GetCurrentUnixMilliseconds() int64 { - return time.Now().UnixNano() / int64(time.Millisecond) + return time.Now().UnixMilli() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exercises/solutions/36_epoch/epoch.go(1 hunks)
🔇 Additional comments (6)
internal/exercises/solutions/36_epoch/epoch.go (6)
10-12: LGTM!Correct implementation for retrieving Unix epoch in seconds.
20-22: LGTM!Correct implementation for retrieving Unix epoch in nanoseconds.
32-34: LGTM, but note the literal timezone format.The function correctly formats the time. The
+0000 UTCis literal text in the format string rather than dynamic timezone formatting. This appears intentional based on the PR requirements showing the same format.
40-43: LGTM!Correct implementation for converting Unix epoch seconds to formatted time string.
26-28: Fix the microsecond format string.The format string
"2006-01-02 15:04:05.000000 +0000 UTC"will literally print ".000000" instead of actual microseconds. In Go's time formatting, you need to use.999999or.000000000(nanoseconds) and truncate.To display microseconds, use this approach:
func GetCurrentFormattedTime() string { - return time.Now().UTC().Format("2006-01-02 15:04:05.000000 +0000 UTC") + now := time.Now().UTC() + return now.Format("2006-01-02 15:04:05.999999") + " +0000 UTC" }Note:
.999999will display microseconds, omitting trailing zeros. For fixed-width microseconds with leading zeros, you'd need to manually format usingnow.Nanosecond() / 1000.Likely an incorrect or invalid review comment.
1-74: Review comment is incorrect - solution tests pass successfully.The original review states tests are not passing, but verification confirms the solution code passes all tests:
==> 36_epoch: Epoch Conversion (solution) PASSED 36_epoch (solution)The test failures mentioned occur in the
templates/directory, which contains intentionally incomplete stub implementations for learners to complete. Thesolutions/directory contains the reference solution, and bothEpochToTimeandTimeToEpochfunctions are correctly implemented and pass their tests.Note: The
NowFormatsfunction (which uses aformatInthelper) is not covered by the current test suite. WhileformatInthas a logic issue (treats all inputs as nanoseconds), this doesn't affect the tested functionality.Likely an incorrect or invalid review comment.
| import ( | ||
| "time" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add fmt import for numeric string conversions.
To implement the fixes suggested for NowFormats(), you'll need to add the fmt package import.
import (
+ "fmt"
"time"
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import ( | |
| "time" | |
| ) | |
| import ( | |
| "fmt" | |
| "time" | |
| ) |
🤖 Prompt for AI Agents
In internal/exercises/solutions/36_epoch/epoch.go around lines 3 to 5, the code
needs the fmt package imported to support numeric-to-string conversions used in
NowFormats(); add fmt to the import block (e.g., include "fmt" alongside "time")
so the formatting functions compile and can convert numbers to strings as
required.
| func TimeToEpoch(input string) int64 { | ||
| t, err := time.ParseInLocation("2006-01-02 15:04:05", input, time.UTC) | ||
| if err != nil { | ||
| return 0 | ||
| } | ||
| return t.Unix() | ||
| } |
There was a problem hiding this comment.
Error handling creates ambiguity.
Returning 0 on parse errors is problematic because 0 is a valid Unix epoch (January 1, 1970). Callers cannot distinguish between a parsing error and the actual epoch zero.
Consider one of these approaches:
Option 1: Return error as second value (idiomatic Go):
-func TimeToEpoch(input string) int64 {
+func TimeToEpoch(input string) (int64, error) {
t, err := time.ParseInLocation("2006-01-02 15:04:05", input, time.UTC)
if err != nil {
- return 0
+ return 0, err
}
- return t.Unix()
+ return t.Unix(), nil
}Option 2: Use sentinel value (if constrained by test signature):
func TimeToEpoch(input string) int64 {
t, err := time.ParseInLocation("2006-01-02 15:04:05", input, time.UTC)
if err != nil {
- return 0
+ return -1 // or panic/log the error
}
return t.Unix()
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
internal/exercises/solutions/36_epoch/epoch.go lines 47-53: The function
currently returns 0 on parse errors which is ambiguous because 0 is a valid Unix
epoch; change the signature to return (int64, error), parse the time and on
success return t.Unix(), nil, and on failure return 0 and the parse error, then
update any callers/tests to handle the error; if changing signature is not
possible, return a documented sentinel (e.g. -1) on error and ensure callers
check for that sentinel.
| func NowFormats() map[string]string { | ||
| now := time.Now().UTC() | ||
| return map[string]string{ | ||
| "UnixSeconds": time.Unix(now.Unix(), 0).Format("2006-01-02 15:04:05") + " (epoch: " + | ||
| time.Unix(now.Unix(), 0).UTC().Format("2006-01-02 15:04:05") + ")", | ||
| "UnixSecondsRaw": time.Unix(now.Unix(), 0).UTC().Format("2006-01-02 15:04:05"), | ||
| "UnixSecondsInt": formatInt(now.Unix()), | ||
| "UnixMilliseconds": formatInt(now.UnixMilli()), | ||
| "UnixNanoseconds": formatInt(now.UnixNano()), | ||
| "FormattedFull": GetCurrentFormattedTime(), | ||
| "FormattedSimple": GetCurrentFormattedTimeSimple(), | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical logic error in NowFormats.
The UnixSecondsInt, UnixMilliseconds, and UnixNanoseconds entries incorrectly use formatInt() which converts the numeric epoch values back into date strings. These should return the raw numeric values as strings.
Apply this diff:
func NowFormats() map[string]string {
now := time.Now().UTC()
return map[string]string{
- "UnixSeconds": time.Unix(now.Unix(), 0).Format("2006-01-02 15:04:05") + " (epoch: " +
- time.Unix(now.Unix(), 0).UTC().Format("2006-01-02 15:04:05") + ")",
- "UnixSecondsRaw": time.Unix(now.Unix(), 0).UTC().Format("2006-01-02 15:04:05"),
- "UnixSecondsInt": formatInt(now.Unix()),
- "UnixMilliseconds": formatInt(now.UnixMilli()),
- "UnixNanoseconds": formatInt(now.UnixNano()),
+ "UnixSeconds": now.Format("2006-01-02 15:04:05 +0000 UTC"),
+ "UnixSecondsRaw": now.Format("2006-01-02 15:04:05"),
+ "UnixSecondsInt": fmt.Sprintf("%d", now.Unix()),
+ "UnixMilliseconds": fmt.Sprintf("%d", now.UnixMilli()),
+ "UnixNanoseconds": fmt.Sprintf("%d", now.UnixNano()),
"FormattedFull": GetCurrentFormattedTime(),
"FormattedSimple": GetCurrentFormattedTimeSimple(),
}
}You'll also need to import fmt at the top of the file.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In internal/exercises/solutions/36_epoch/epoch.go around lines 57 to 69, the
entries for UnixSecondsInt, UnixMilliseconds, and UnixNanoseconds incorrectly
call formatInt (which converts epoch values to date strings); replace those
calls with fmt.Sprintf("%d", <numeric>) to return the raw numeric epoch values
as strings (use now.Unix(), now.UnixMilli(), and now.UnixNano() respectively),
and add an import for "fmt" at the top of the file.
| func formatInt(val int64) string { | ||
| return time.Unix(0, val).UTC().Format("2006-01-02 15:04:05") | ||
| } |
There was a problem hiding this comment.
Remove or fix the formatInt helper.
This helper incorrectly treats all int64 values as nanoseconds in time.Unix(0, val). This is fundamentally wrong when called with epoch seconds or milliseconds. Based on the fix suggested for NowFormats(), this helper should be removed entirely.
-// Helper: format int64 as string
-func formatInt(val int64) string {
- return time.Unix(0, val).UTC().Format("2006-01-02 15:04:05")
-}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func formatInt(val int64) string { | |
| return time.Unix(0, val).UTC().Format("2006-01-02 15:04:05") | |
| } |
🤖 Prompt for AI Agents
internal/exercises/solutions/36_epoch/epoch.go around lines 72 to 74, the
formatInt helper incorrectly treats the int64 argument as nanoseconds
(time.Unix(0, val)) which is wrong for epoch seconds/milliseconds and the helper
should be removed; delete the formatInt function and replace all its callsites
with explicit, correct conversions and formatting (e.g., use time.Unix(seconds,
0).UTC().Format(...) for epoch seconds or time.Unix(0,
milliseconds*int64(time.Millisecond)).UTC().Format(...) for ms) or use the same
corrected approach used in NowFormats(), ensuring each call uses the proper unit
before formatting.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/exercises/solutions/36_epoch/epoch.go (1)
14-17: Consider using UnixMilli() for simplicity.The manual conversion from nanoseconds to milliseconds is correct but verbose. Go's standard library provides
time.Now().UnixMilli()which is more idiomatic and clearer.Apply this diff:
-// GetCurrentUnixMilliseconds returns the current time as a Unix epoch in milliseconds (int64). -func GetCurrentUnixMilliseconds() int64 { - return time.Now().UnixNano() / int64(time.Millisecond) -} +// GetCurrentUnixMilliseconds returns the current time as a Unix epoch in milliseconds (int64). +func GetCurrentUnixMilliseconds() int64 { + return time.Now().UnixMilli() +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exercises/solutions/36_epoch/epoch.go(1 hunks)
🔇 Additional comments (5)
internal/exercises/solutions/36_epoch/epoch.go (5)
3-5: Missing fmt import for proper numeric conversions.The
NowFormats()function will require thefmtpackage when the critical issues withformatInt()are addressed. The numeric epoch values should be converted to strings usingfmt.Sprintf("%d", ...)rather than the current incorrect approach.
38-43: LGTM!The epoch-to-time conversion correctly uses
time.Unix()with UTC normalization and the appropriate format string.
47-53: Error handling creates ambiguity with valid epoch zero.Returning
0on parse errors is problematic because0represents a valid Unix timestamp (January 1, 1970, 00:00:00 UTC). Callers cannot distinguish between a genuine parse error and the actual epoch zero.The idiomatic Go approach would return
(int64, error), but if the function signature is constrained by the exercise tests, document the limitation clearly or use a sentinel value like-1with appropriate documentation.
57-69: Critical logic error: numeric epochs incorrectly formatted as date strings.The
NowFormats()function has fundamental issues:
- Lines 60-62: Redundant round-trip conversions (
now.Unix()→time.Unix(now.Unix(), 0)) that serve no purpose.- Lines 63-65: The
formatInt()calls incorrectly convert numeric epoch values back into date strings, when they should return the raw numbers as strings (e.g., "1351700038", "1351700038292").Per the PR objectives, the maintainer explicitly requested multiple formats including raw Unix timestamps as numbers. The current implementation defeats this purpose by converting them all to date strings.
72-74: Remove fundamentally flawed formatInt helper.This helper incorrectly assumes all int64 values are nanoseconds by using
time.Unix(0, val). This produces completely wrong results when called with epoch seconds or milliseconds (which is the case in lines 63-64).The helper should be removed entirely and replaced with proper conversions:
- For numeric epoch values →
fmt.Sprintf("%d", value)- For epoch seconds to time →
time.Unix(seconds, 0).UTC().Format(...)- For epoch milliseconds to time →
time.UnixMilli(ms).UTC().Format(...)
|
@Omesh2004 Thank you so much for your time!! Appreciate your efforts and time to contribute towards the project ⭐✨ |
zhravan
added
the
minor
2 participants
Summary
i added epoch template .I wanted to start of with the basics of go
Checklist
make verifyorgolearn verify <slug>Paste before/after where helpful.
Related issues
Fixes #
Summary by CodeRabbit
New Features
Tests
Chores