Register cycle kinds explicitly - #91
Merged
Merged
Conversation
Tracking of handler classes moves out of Cycle and into a registry, and subclassing no longer registers anything. A class joins by declaring what it handles: handles :lookback, notation: "L", periods: %w[D W M Y] which sets the kind, notation id, periods and volume-only flag and registers in one step, so declaration and registration cannot drift apart. Inheriting from Cycle for any other reason — an abstract intermediate, a test double — now carries no hidden side effect. The parser's pattern is built from the registry rather than written out, so an application can declare a kind of its own and have its notation parse without reopening this gem. Registering a class displaces any already answering to the same kind or notation id, which is what lets an application replace a built-in rather than only add alongside it. TimeSpan::DatePeriod keeps its own inherited hook, and period keys remain a closed set; both are left for a follow-up. Added: `Cycle.handles` declares and registers the kind a class handles Added: `SOF::CycleRegistry`, reachable as `Cycle.registry`, so an application can add or replace a cycle kind Changed: cycle classes register by declaring rather than by subclassing; `Cycle.cycle_handlers` is gone Removed: `Parser::PARTS_REGEX`, replaced by `Parser.parts_regex` built from the registered kinds Version: minor
saturnflyer
force-pushed
the
feature/cycle-registry
branch
from
July 27, 2026 18:39
b075e78 to
c0b6f0f
Compare
DatePeriod tracked its subclasses through an inherited hook, the same pattern Cycle had, and the parser's period alternation was written out as D|W|M|Q|Y. Periods now declare what they measure: measures :month, code: "M", interval: "months" which registers them, and the parser builds the period alternation from the registry alongside the kind alternation. Subclassing DatePeriod on its own registers nothing. Unlike cycle kinds, periods are not an extension point: DatePeriod is a private constant, so an application cannot name it to subclass it. The registry is private for the same reason, reached through the public TimeSpan.period_registry — which is what the parser uses, rather than reaching into DatePeriod for it. Also moves the cycle registry from SOF::CycleRegistry to SOF::Cycle::Registry. It was introduced a commit ago as a third constant directly under SOF, which undid part of what restricting the namespace set out to do. Added: `DatePeriod.measures` declares and registers the period a class measures Changed: period classes register by declaring rather than by subclassing; the parser's period alternation is built from the registry Changed: `SOF::CycleRegistry` is now `SOF::Cycle::Registry`
saturnflyer
force-pushed
the
feature/cycle-registry
branch
from
July 27, 2026 18:54
e46664f to
3c41c8f
Compare
Registration never required inheritance — the registry takes any class answering the same questions — but the only way to reach it was Cycle.registry.register, which reads as internal plumbing and documents nothing about what a handler must provide. Cycle.register and Cycle.unregister name it, and the contract a non-inheriting handler has to satisfy is written down beside them. Inheriting and declaring with `handles` remains the easier path, since it supplies the behaviour Cycle.for goes on to use. Added: `Cycle.register` and `Cycle.unregister` register a handler that need not inherit from Cycle
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.
Why
Redoes the spirit of #55 — move tracking of cycle classes out of
Cycleand into a registry — with one change of goal: noinheritedhook.#55 kept it, just pointed it at the registry:
That leaves registration as a side effect of subclassing. Anything inheriting from
Cycle— an abstract intermediate, a test double, a one-off — silently becomes a handler, and there's no way to inherit without joining.The larger problem it exposed: the gem was extensible in the half that didn't matter and closed in the half that did. An application could define its own cycle class and it registered fine, but the notation would not parse:
because the parser's kind alternation was a closed literal in a constant:
So adding a kind meant patching
PARTS_REGEX. A registry that only replacesinheritedis a tidy-up; a registry the parser reads is the thing that makes the extension point real.Summary
Cycle.handlesdeclares kind, notation id, periods and the volume-only flag, and registers — one call, so declaration and registration cannot drift apart. It replaces four bare ivar assignments per handler, which nothing previously enforced.SOF::CycleRegistry, reachable asCycle.registry, owns the collection.Cycle.inheritedandCycle.cycle_handlersare gone.unregisterundoes it.Regexp.escaped and ordered longest-first when building the alternation.An application now gets this, with no change to the gem:
Notes for reviewers
Cyclekeeps its own@kind/@notation_id/@valid_periods/@volume_onlydefaults as plain assignments, since the base class handles no kind and must not register itself.Parser.dormant_capable_kindsstill has to be a method rather than a constant, for the same load-order reason as before: handlers register assof/cycles/*.rbloads, which happens afterparser.rb.Covered by
spec/sof/cycle_registry_spec.rb(registry behaviour in isolation) andspec/sof/custom_cycle_kind_spec.rb(an application adding a kind, overriding a built-in, and subclassing without declaring). The latter restores the registry around each example so registration cannot leak between specs.Periods too
DatePeriodhad the identical pattern one level down — aninheritedhook, and aD|W|M|Q|Yalternation written out in the regex — so a second commit gives it the same treatment:The parser now builds the period alternation from the registry alongside the kind alternation. No
inheritedhook remains anywhere inlib/.Periods are not an extension point, unlike kinds.
DatePeriodis aprivate_constant, so an application cannot name it in order to subclass it — the asymmetry withSOF::Cycle, which is public. The registry is private for the same reason and is reached through the publicTimeSpan.period_registry, which is what the parser uses rather than reaching intoDatePeriodfor it. Opening periods up would mean makingDatePeriodpublic API; that's a deliberate decision for another day, not a side effect of this change.That commit also moves the cycle registry from
SOF::CycleRegistrytoSOF::Cycle::Registry. The first commit introduced it as a third constant directly underSOF, which undid part of what #7 set out to do.SOF.constantsis back to[:Cycle, :Cycles].Deliberately not included
Thread safety. #55 reached for
Concurrent::Setand aconcurrent-rubydependency. Both registries are written at load time and read afterwards, so a plain collection is enough. The genuinely racy state isTimeSpan's cached periods and theDatePeriodmemo hashes — a real issue, but a separate one.