PTR-3591 Added modTimeCursor along with encode/decode functions#97
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds a modtimeCursor in rblob to track S3 listing progress by object modification time. It defines the cursor struct (Key, ModTime), a String() method that serialises as "key|unixNano" (empty key → empty string), and parseModTimeCursor which validates and parses that format into UTC time. Tests cover String() edge cases, parser error cases, and round-trip correctness. Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🧹 Nitpick comments (1)
rblob/modTimeBlob_internal_test.go (1)
37-41: ⚡ Quick winAdd parser round-trip coverage for pipe-containing keys (and empty-key rejection).
You already test
String()withfile|with|pipes, but not parse/round-trip for that case. Add a round-trip assertion for pipe-containing keys and a parse failure case for|123to prevent future regressions.Also applies to: 78-90
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rblob/modTimeBlob_internal_test.go` around lines 37 - 41, Add two test cases in modTimeBlob_internal_test.go: one asserting round-trip for a pipe-containing key by creating modtimeCursor{Key: "file|with|pipes", ModTime: time.Unix(0,42)}, calling its String() and then the parser (the function that parses cursor strings, e.g. ParseModtimeCursor/ParseCursor) and asserting the parsed value equals the original; and a negative test asserting that parsing the string "|123" returns an error (i.e. parser rejects empty key). Use the existing modtimeCursor type and its String() method and the project’s cursor-parse function name to locate where to add these assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rblob/modTimeBlob.go`:
- Around line 60-67: The cursor parsing currently uses strings.Cut on the first
'|' which mis-parses keys containing '|' and allows an empty key for non-empty
cursors; update the parsing in the modtimeCursor unmarshalling logic (where key,
rest, found := strings.Cut(s, "|") is used) to locate the last separator (e.g.,
strings.LastIndex or LastIndexByte), split into key = s[:idx] and rest =
s[idx+1:], return an error if no separator is found, and additionally reject
cases where key == "" but rest != "" (i.e., non-empty cursor with empty key)
before calling strconv.ParseInt on rest to produce the modtimeCursor.
---
Nitpick comments:
In `@rblob/modTimeBlob_internal_test.go`:
- Around line 37-41: Add two test cases in modTimeBlob_internal_test.go: one
asserting round-trip for a pipe-containing key by creating modtimeCursor{Key:
"file|with|pipes", ModTime: time.Unix(0,42)}, calling its String() and then the
parser (the function that parses cursor strings, e.g.
ParseModtimeCursor/ParseCursor) and asserting the parsed value equals the
original; and a negative test asserting that parsing the string "|123" returns
an error (i.e. parser rejects empty key). Use the existing modtimeCursor type
and its String() method and the project’s cursor-parse function name to locate
where to add these assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e960abd-e1e4-4810-829f-f697d95ad655
📒 Files selected for processing (2)
rblob/modTimeBlob.gorblob/modTimeBlob_internal_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rblob/modTimeBlob_internal_test.go (1)
51-82: ⚡ Quick winConsider adding an explicit test case for parsing keys containing pipes.
Whilst
TestModtimeCursor_Stringverifies serialisation of keys with pipes (line 38-41),TestParseModTimeCursorlacks a corresponding parse test. Adding an explicit test case such asparseModTimeCursor("file|with|pipes|42")would strengthen coverage of theLastIndexlogic and provide clearer failure messages if the implementation changes.✅ Suggested test case
t.Run("key containing pipes parses correctly", func(t *testing.T) { c, err := parseModTimeCursor("file|with|pipes|42") jtest.RequireNil(t, err) require.Equal(t, "file|with|pipes", c.Key) require.Equal(t, time.Unix(0, 42).UTC(), c.ModTime) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rblob/modTimeBlob_internal_test.go` around lines 51 - 82, Add a test within TestParseModTimeCursor that verifies parseModTimeCursor correctly handles keys containing pipe characters: call parseModTimeCursor("file|with|pipes|42"), assert no error, assert c.Key == "file|with|pipes" and c.ModTime == time.Unix(0, 42).UTC(); this mirrors the serialization check in TestModtimeCursor_String and ensures the LastIndex split logic in parseModTimeCursor handles multiple pipes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@rblob/modTimeBlob_internal_test.go`:
- Around line 51-82: Add a test within TestParseModTimeCursor that verifies
parseModTimeCursor correctly handles keys containing pipe characters: call
parseModTimeCursor("file|with|pipes|42"), assert no error, assert c.Key ==
"file|with|pipes" and c.ModTime == time.Unix(0, 42).UTC(); this mirrors the
serialization check in TestModtimeCursor_String and ensures the LastIndex split
logic in parseModTimeCursor handles multiple pipes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c6b0dfb-c414-40ee-aaed-66836ed300a1
📒 Files selected for processing (2)
rblob/modTimeBlob.gorblob/modTimeBlob_internal_test.go
|
3 participants
For External Submissions:
Addresses Issue #98
Summary by CodeRabbit
New Features
Tests