Review follow-ups: P&L doc clarifications and slices.Sort - #3
Merged
Conversation
Make three behaviours explicit in the README so they are stated rather than discovered: - the exporter owns which deals count and the sign convention, and the CLI does not re-check them — keep the two in step; - dates, --last and "today" are interpreted in UTC and deals bucket by UTC day, which can differ from local day from a far-offset timezone; - --last runs from N units ago through today inclusive, so 30d covers 31 calendar days. No behaviour change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
slices.Sort works on any ordered type, so the int64 less-closure in sort.Slice is no longer needed and sort.Strings becomes slices.Sort. Behaviour is unchanged; covered by the existing aggregate tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Both fields are kept for schema fidelity with the exporter; cash flows are deliberately excluded from trading P&L and open positions are reserved for a future command. The comment stops a future tidy-up from deleting them and reintroducing a cash-flow bug or breaking the schema contract. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3 +/- ##
=======================================
Coverage 78.65% 78.65%
=======================================
Files 11 11
Lines 431 431
=======================================
Hits 339 339
Misses 75 75
Partials 17 17 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Small follow-ups from a full-codebase review. Three focused commits; no behaviour change to any command.
Changes
docs:clarify P&L semantics in the README — make three existing behaviours explicit rather than discoverable:--lastand "today" are interpreted in UTC and deals bucket by UTC day — which can differ from local day from a far-offset timezone (e.g. NZ) near midnight.--lastruns from N units ago through today inclusive, so30dcovers 31 calendar days.refactor:sort withslicesover thesortpackage —slices.Sortworks on any ordered type, so theint64less-closure insort.Slicedisappears andsort.Stringsbecomesslices.Sort. Behaviour unchanged; covered by existing aggregate tests.docs:explain the decoded-but-unused fields — a comment onOpenPositions/CashFlowsrecords that they're kept for schema fidelity, that cash flows are deliberately excluded from trading P&L, and that open positions are reserved for a future command — so a future tidy-up doesn't delete them and reintroduce a cash-flow bug.Decisions explicitly not taken
The review also considered a runtime guard on the exporter contract, a
--tzflag, changing--last Ndto exactly N days, switching%v→%w, reconciling rounded rows to the total, and extracting a shared-flags helper. Each was deliberately declined as premature or incorrect for this tool; see commit messages / review notes.Verification
go build,go vet,gofmt -d, andgo test ./...all clean after every change.🤖 Generated with Claude Code