feat(reports): close the report granularity vocabulary - #6
Merged
Conversation
Report granularity is monthly or weekly and nothing else, but C7 never said so. The set exists only as a `'monthly' | 'weekly'` docblock repeated across five classes, and the value is stored inside a saved report's definition payload where no column constraint can reach it. This is the third vocabulary in the codebase that says `weekly`. The series cadence of C2-R20 and the digest cadence of C8-R22 are already closed and already required to be distinct from each other; granularity was left out of that pass. Sharing a type between any two of them would be wrong in a way that type-checks: a report is not on a cadence, and widening its buckets has nothing to do with how often a series recurs or how often a digest is sent. C7-R21 closes the set, requires one named type rather than free strings, requires a stored value outside the set to be rejected rather than defaulted, and states that the type MUST NOT be shared with either cadence. Quarterly is deliberately not in the vocabulary. The generator reaches it on its own when a range would exceed the point cap, but the user never picks it and a saved report never holds it — it is a widening outcome, not a choice. The prose says this explicitly, because the existing widening paragraph names quarterly and would otherwise read as if it were selectable. Spec: GOV-R13 Signed-off-by: Wessel Verheij <info@nightworks.io>
|
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
1 participant
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.
Spec: GOV-R13
Summary
Adds C7-R21, closing the report granularity vocabulary to
monthlyandweekly, requiring one named type rather than free strings, and stating thatthe type must not be shared with the series cadence (C2) or the digest cadence
(C8).
Why
Granularity is monthly or weekly and nothing else, but C7 never said so. The
set exists only as a
'monthly' | 'weekly'docblock repeated across fiveclasses, and the value is stored inside a saved report's
definitionJSONpayload — so unlike the series cadence there is no column a CHECK constraint
could reach. The requirement is worded for that: it asks for rejection of an
out-of-set value rather than for a column constraint.
This is the third vocabulary that says
weekly. C2-R20 and C8-R22 alreadyclose the other two and already require them to be distinct from each other;
granularity was left out of that pass. Sharing a type between any two of them
would be wrong in a way that still type-checks — a report is not on a cadence,
and widening its buckets has nothing to do with how often a series recurs or
how often a digest is sent.
Note on quarterly
Quarterly is deliberately not in the vocabulary.
TimeBucketGeneratorreaches it on its own when a range would exceed
MAX_BUCKET_POINTS, but theuser never selects it and a saved report never stores it — it is a widening
outcome, not a choice.
This needed saying in prose, because the existing "Time buckets widen rather
than truncate" paragraph names quarterly and, read alone, suggests it is
selectable. Someone implementing R21 from the requirement table plus that
paragraph could reasonably have put a third case in the enum.
Verification
python3 scripts/integrity.py— cleanc2-recurring.md,c8-notifications.md), addedto Related as well as inline
Follow-up
The code change lands separately and cites C7-R21: a
ReportGranularityenumreplacing the string on
ReportDefinition,TimeBucketGenerator,TimeBucketSpendQuery,NetWorthSeriesQueryandReportBuilder.