From 6f562e72687e09395dab61e1db15cb5b040ff042 Mon Sep 17 00:00:00 2001 From: Paul Buetow Date: Sat, 23 May 2026 21:16:14 +0300 Subject: refactor: reduce GetMetricRegistry() global singleton usage (DIP) Dependency Inversion: scope MetricRegistry as a dependency rather than relying on a global singleton. This enables testing with custom registries and decouples code from global mutable state. Changes: - NewRPN(vars, reg...): accepts optional *MetricRegistry parameter. Defaults to GetMetricRegistry() when not provided (backward compatible). - NewOperations(vars, reg...): accepts optional *MetricRegistry parameter. Defaults to GetMetricRegistry() when not provided (backward compatible). - parseNumberWithMetric(token, reg): requires *MetricRegistry parameter instead of calling GetMetricRegistry() internally. - RPN struct stores metricRegistry field, used by rpn_parse.go for metric lookups (@ prefix and parseNumberWithMetric). - Added Operations.MetricRegistry() getter for external access. All existing callers remain backward compatible via variadic defaults. Tests updated to pass GetMetricRegistry() to parseNumberWithMetric. --- internal/repl/signal_test.go | 9 ++++---- internal/rpn/metric_parse.go | 6 +++--- internal/rpn/metric_test.go | 6 +++--- internal/rpn/operations.go | 14 +++++++++++-- internal/rpn/rpn_parse.go | 4 ++-- internal/rpn/rpn_state.go | 49 +++++++++++++++++++++++++------------------- 6 files changed, 53 insertions(+), 35 deletions(-) diff --git a/internal/repl/signal_test.go b/internal/repl/signal_test.go index 7d5e72e..ac5d4b4 100644 --- a/internal/repl/signal_test.go +++ b/internal/repl/signal_test.go @@ -6,6 +6,7 @@ package repl import ( "os" "sync" + "sync/atomic" "syscall" "testing" "time" @@ -96,9 +97,9 @@ func TestSignalHandlerStartCallbackRunsInGoroutine(t *testing.T) { func TestSignalHandlerSingleShot(t *testing.T) { h := NewSignalHandler() - var callbackCount int + var callbackCount atomic.Int32 h.Start(func() { - callbackCount++ + callbackCount.Add(1) }) // First signal should trigger callback @@ -112,7 +113,7 @@ func TestSignalHandlerSingleShot(t *testing.T) { // Stop() unregisters the signal channel. h.Stop() - if callbackCount != 1 { - t.Errorf("expected callback count 1, got %d", callbackCount) + if callbackCount.Load() != 1 { + t.Errorf("expected callback count 1, got %d", callbackCount.Load()) } } diff --git a/internal/rpn/metric_parse.go b/internal/rpn/metric_parse.go index 8a00a0d..be6df93 100644 --- a/internal/rpn/metric_parse.go +++ b/internal/rpn/metric_parse.go @@ -7,10 +7,10 @@ import "strconv" // parseNumberWithMetric attempts to split a token into a number and a metric suffix. // E.g., "100Mbps" -> (100, Mbps), "5.5GB" -> (5.5, GB), "2hr" -> (2, hr). -// The metric suffix is looked up in the global registry (exact, alias, then case-insensitive). +// The metric suffix is looked up in the given registry (exact, alias, then case-insensitive). // Returns (num, metric, true) if successful, or (0, nil, false) if the token // does not contain a number+metric combination. -func parseNumberWithMetric(token string) (float64, *Metric, bool) { +func parseNumberWithMetric(token string, reg *MetricRegistry) (float64, *Metric, bool) { if len(token) == 0 { return 0, nil, false } @@ -73,7 +73,7 @@ func parseNumberWithMetric(token string) (float64, *Metric, bool) { return 0, nil, false } - metric, ok := GetMetricRegistry().FindWithAliases(metricName) + metric, ok := reg.FindWithAliases(metricName) if !ok { return 0, nil, false } diff --git a/internal/rpn/metric_test.go b/internal/rpn/metric_test.go index 7441705..48c6e91 100644 --- a/internal/rpn/metric_test.go +++ b/internal/rpn/metric_test.go @@ -729,7 +729,7 @@ func TestParseNumberWithMetric(t *testing.T) { } for _, tt := range tests { - num, metric, ok := parseNumberWithMetric(tt.token) + num, metric, ok := parseNumberWithMetric(tt.token, GetMetricRegistry()) if ok != tt.wantOK { t.Errorf("parseNumberWithMetric(%q) ok = %v, want %v", tt.token, ok, tt.wantOK) continue @@ -773,7 +773,7 @@ func TestParseNumberWithMetricAliases(t *testing.T) { } for _, tt := range tests { - _, metric, ok := parseNumberWithMetric(tt.token) + _, metric, ok := parseNumberWithMetric(tt.token, GetMetricRegistry()) if !ok { t.Fatalf("parseNumberWithMetric(%q) = false, want true", tt.token) } @@ -786,7 +786,7 @@ func TestParseNumberWithMetricAliases(t *testing.T) { func TestParseNumberWithMetricExactMatch(t *testing.T) { // Bps (capital B) should NOT resolve to bps - _, _, ok := parseNumberWithMetric("100Bps") + _, _, ok := parseNumberWithMetric("100Bps", GetMetricRegistry()) if ok { t.Error("parseNumberWithMetric(100Bps) should fail (B = bytes)") } diff --git a/internal/rpn/operations.go b/internal/rpn/operations.go index caed87a..64a0d2d 100644 --- a/internal/rpn/operations.go +++ b/internal/rpn/operations.go @@ -198,14 +198,19 @@ type Operations struct { var _ Operator = (*Operations)(nil) // NewOperations creates a new Operations instance with the given variable store. -func NewOperations(vars VariableStore) *Operations { +// If no registry is provided, defaults to the global MetricRegistry. +func NewOperations(vars VariableStore, reg ...*MetricRegistry) *Operations { consts := NewConstants() + r := GetMetricRegistry() + if len(reg) > 0 && reg[0] != nil { + r = reg[0] + } return &Operations{ vars: vars, consts: consts, mode: FloatMode, // default prefixMode: SI, // default - metricRegistry: GetMetricRegistry(), + metricRegistry: r, } } @@ -241,6 +246,11 @@ func (o *Operations) SetPrefixMode(mode PrefixMode) { o.prefixMode = mode } +// MetricRegistry returns the metric registry used by this Operations instance. +func (o *Operations) MetricRegistry() *MetricRegistry { + return o.metricRegistry +} + // OperatorHandler represents a function that handles an operator. // Returns (result string, handled bool, error error). // result is non-empty only for commands that return immediately (like show, vars). diff --git a/internal/rpn/rpn_parse.go b/internal/rpn/rpn_parse.go index 3633db9..ef7be53 100644 --- a/internal/rpn/rpn_parse.go +++ b/internal/rpn/rpn_parse.go @@ -327,7 +327,7 @@ func (r *RPN) evaluate(input string, tokens []string) (string, error) { } // Check if it's a number with a metric suffix (e.g., 100Mbps, 5.5GB, 2hr) - if num, metric, ok := parseNumberWithMetric(token); ok { + if num, metric, ok := parseNumberWithMetric(token, r.metricRegistry); ok { if stack.Len() >= r.maxStack { return "", fmt.Errorf("stack overflow") } @@ -339,7 +339,7 @@ func (r *RPN) evaluate(input string, tokens []string) (string, error) { // Pushes a Number with value 1 and the looked-up metric if len(token) > 1 && token[0] == '@' { metricName := token[1:] - if metric, ok := GetMetricRegistry().FindWithAliases(metricName); ok { + if metric, ok := r.metricRegistry.FindWithAliases(metricName); ok { if stack.Len() >= r.maxStack { return "", fmt.Errorf("stack overflow") } diff --git a/internal/rpn/rpn_state.go b/internal/rpn/rpn_state.go index a27f63a..5de57c4 100644 --- a/internal/rpn/rpn_state.go +++ b/internal/rpn/rpn_state.go @@ -11,31 +11,38 @@ import ( // It is thread-safe for concurrent read operations, but write operations // on the stack or mode should be synchronized externally or use the provided methods. type RPN struct { - mu sync.RWMutex - vars VariableStore - consts ConstantsProvider - ops Operator - opRegistry *OperatorRegistry - assignHandler *assignmentHandler - maxStack int - currentStack *Stack - mode CalculationMode + mu sync.RWMutex + vars VariableStore + consts ConstantsProvider + ops Operator + opRegistry *OperatorRegistry + assignHandler *assignmentHandler + maxStack int + currentStack *Stack + mode CalculationMode + metricRegistry *MetricRegistry } // NewRPN creates a new RPN parser and evaluator with the given variable store. -func NewRPN(vars VariableStore) *RPN { +// If no registry is provided, defaults to the global MetricRegistry. +func NewRPN(vars VariableStore, reg ...*MetricRegistry) *RPN { consts := NewConstants() - ops := NewOperations(vars) + r := GetMetricRegistry() + if len(reg) > 0 && reg[0] != nil { + r = reg[0] + } + ops := NewOperations(vars, r) ops.SetMode(FloatMode) // Set default mode return &RPN{ - vars: vars, - consts: consts, - ops: ops, - opRegistry: NewOperatorRegistry(ops), - assignHandler: newAssignmentHandler(), - maxStack: 1000, // Reasonable limit for RPN expressions - currentStack: NewStack(), - mode: FloatMode, // Default mode + vars: vars, + consts: consts, + ops: ops, + opRegistry: NewOperatorRegistry(ops), + assignHandler: newAssignmentHandler(), + maxStack: 1000, // Reasonable limit for RPN expressions + currentStack: NewStack(), + mode: FloatMode, // Default mode + metricRegistry: r, } } @@ -46,7 +53,7 @@ func (r *RPN) GetConstants() ConstantsProvider { } // GetMode returns the current calculation mode. -// This method is thread-safe for concurrent reads. +// This method is thread-safe for reads. func (r *RPN) GetMode() CalculationMode { r.mu.RLock() defer r.mu.RUnlock() @@ -104,7 +111,7 @@ func (r *RPN) SetPrefixMode(mode PrefixMode) { // GetPrefixMode returns the current prefix mode. // Delegates to the Operations instance. -// This method is thread-safe for concurrent reads. +// This method is thread-safe for reads. func (r *RPN) GetPrefixMode() PrefixMode { r.mu.RLock() defer r.mu.RUnlock() -- cgit v1.2.3