From 19e88c1ed7ecde966372adefeb3d468374ef1dd7 Mon Sep 17 00:00:00 2001 From: Craig Jennings Date: Sat, 8 Nov 2025 11:33:31 -0600 Subject: refactor: config: replace magic numbers with named constants MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace two magic numbers with self-documenting constants: 1. Cache cleanup percentage (line 395): - Before: (dotimes (_ (/ (length entries) 5)) - After: (dotimes (_ (floor (* (length entries) wttrin--cache-cleanup-percentage))) - Added: wttrin--cache-cleanup-percentage constant (0.20 = 20%) - Explains WHY 20%: provides buffer before next cleanup cycle 2. Mode-line startup delay (line 508): - Before: (run-at-time 3 nil #'wttrin--mode-line-fetch-weather) - After: (run-at-time wttrin-mode-line-startup-delay nil ...) - Added: wttrin-mode-line-startup-delay defcustom (default 3 seconds) - Now user-customizable, range 1-5 seconds recommended Added comprehensive tests (8 tests, all passing): - 5 tests verify cache cleanup behavior (removes ~20% of oldest entries) - 3 tests verify startup delay defcustom exists and has reasonable value Benefits: - Self-documenting code (explains WHY these values exist) - Startup delay is now user-customizable - Easier for future maintainers to understand rationale 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- tests/test-wttrin--cleanup-cache-constants.el | 143 ++++++++++++++++++++++++++ wttrin.el | 24 ++++- 2 files changed, 162 insertions(+), 5 deletions(-) create mode 100644 tests/test-wttrin--cleanup-cache-constants.el diff --git a/tests/test-wttrin--cleanup-cache-constants.el b/tests/test-wttrin--cleanup-cache-constants.el new file mode 100644 index 0000000..60d784b --- /dev/null +++ b/tests/test-wttrin--cleanup-cache-constants.el @@ -0,0 +1,143 @@ +;;; test-wttrin--cleanup-cache-constants.el --- Tests for cache cleanup constants -*- lexical-binding: t; -*- + +;; Copyright (C) 2024 Craig Jennings + +;;; Commentary: +;; Unit tests verifying cache cleanup behavior with named constants. +;; These tests verify the behavior before and after refactoring magic numbers. + +;;; Code: + +(require 'ert) +(require 'wttrin) + +;;; Normal Cases - Cache Cleanup Behavior + +(ert-deftest test-wttrin--cleanup-cache-removes-oldest-entries () + "Test that cleanup removes oldest entries when cache exceeds max." + (let ((wttrin--cache (make-hash-table :test 'equal)) + (wttrin-cache-max-entries 5) + (now (float-time))) + ;; Add 10 entries with sequential timestamps + (dotimes (i 10) + (puthash (format "location-%d" i) + (cons (+ now i) (format "data-%d" i)) + wttrin--cache)) + ;; Cache should have 10 entries + (should (= 10 (hash-table-count wttrin--cache))) + + ;; Trigger cleanup + (wttrin--cleanup-cache-if-needed) + + ;; After cleanup, cache should have fewer entries + ;; With 10 entries and max of 5, should remove 20% = 2 entries + ;; Leaving 8 entries + (should (= 8 (hash-table-count wttrin--cache))) + + ;; Oldest entries (location-0, location-1) should be removed + (should-not (gethash "location-0" wttrin--cache)) + (should-not (gethash "location-1" wttrin--cache)) + + ;; Newer entries should remain + (should (gethash "location-9" wttrin--cache)) + (should (gethash "location-8" wttrin--cache)))) + +(ert-deftest test-wttrin--cleanup-cache-removes-approximately-20-percent () + "Test that cleanup removes approximately 20% of entries." + (let ((wttrin--cache (make-hash-table :test 'equal)) + (wttrin-cache-max-entries 10) + (now (float-time))) + ;; Add 20 entries (twice the max) + (dotimes (i 20) + (puthash (format "loc-%d" i) + (cons (+ now i) (format "data-%d" i)) + wttrin--cache)) + + (should (= 20 (hash-table-count wttrin--cache))) + + ;; Trigger cleanup - should remove 20% of 20 = 4 entries + (wttrin--cleanup-cache-if-needed) + + ;; Should have 16 entries remaining (20 - 4) + (should (= 16 (hash-table-count wttrin--cache))))) + +(ert-deftest test-wttrin--cleanup-cache-no-action-when-under-max () + "Test that cleanup does nothing when cache is under max." + (let ((wttrin--cache (make-hash-table :test 'equal)) + (wttrin-cache-max-entries 10) + (now (float-time))) + ;; Add 5 entries (under max of 10) + (dotimes (i 5) + (puthash (format "location-%d" i) + (cons (+ now i) (format "data-%d" i)) + wttrin--cache)) + + (should (= 5 (hash-table-count wttrin--cache))) + + ;; Trigger cleanup + (wttrin--cleanup-cache-if-needed) + + ;; Should still have 5 entries (no cleanup needed) + (should (= 5 (hash-table-count wttrin--cache))))) + +;;; Boundary Cases + +(ert-deftest test-wttrin--cleanup-cache-exactly-at-max () + "Test that cleanup does nothing when cache is exactly at max." + (let ((wttrin--cache (make-hash-table :test 'equal)) + (wttrin-cache-max-entries 5) + (now (float-time))) + ;; Add exactly 5 entries (at max) + (dotimes (i 5) + (puthash (format "location-%d" i) + (cons (+ now i) (format "data-%d" i)) + wttrin--cache)) + + (should (= 5 (hash-table-count wttrin--cache))) + + ;; Trigger cleanup - should do nothing since not > max + (wttrin--cleanup-cache-if-needed) + + ;; Should still have 5 entries + (should (= 5 (hash-table-count wttrin--cache))))) + +(ert-deftest test-wttrin--cleanup-cache-one-over-max () + "Test cleanup when cache has just one entry over max." + (let ((wttrin--cache (make-hash-table :test 'equal)) + (wttrin-cache-max-entries 5) + (now (float-time))) + ;; Add 6 entries (1 over max) + (dotimes (i 6) + (puthash (format "location-%d" i) + (cons (+ now i) (format "data-%d" i)) + wttrin--cache)) + + (should (= 6 (hash-table-count wttrin--cache))) + + ;; Trigger cleanup - should remove 20% of 6 = 1.2 = 1 entry (floor) + (wttrin--cleanup-cache-if-needed) + + ;; Should have 5 entries remaining + (should (= 5 (hash-table-count wttrin--cache))) + + ;; Oldest entry (location-0) should be removed + (should-not (gethash "location-0" wttrin--cache)))) + +;;; Tests for Mode-Line Startup Delay (after refactoring) + +(ert-deftest test-wttrin-mode-line-startup-delay-exists () + "Test that wttrin-mode-line-startup-delay defcustom exists after refactoring." + ;; This test will fail initially, then pass after refactoring + (should (boundp 'wttrin-mode-line-startup-delay))) + +(ert-deftest test-wttrin-mode-line-startup-delay-is-number () + "Test that startup delay is a number." + (should (numberp wttrin-mode-line-startup-delay))) + +(ert-deftest test-wttrin-mode-line-startup-delay-reasonable-range () + "Test that startup delay is in reasonable range (1-10 seconds)." + (should (>= wttrin-mode-line-startup-delay 1)) + (should (<= wttrin-mode-line-startup-delay 10))) + +(provide 'test-wttrin--cleanup-cache-constants) +;;; test-wttrin--cleanup-cache-constants.el ends here diff --git a/wttrin.el b/wttrin.el index 527ac02..afc0d2f 100644 --- a/wttrin.el +++ b/wttrin.el @@ -95,6 +95,12 @@ units (default)." :group 'wttrin :type 'integer) +(defconst wttrin--cache-cleanup-percentage 0.20 + "Percentage of cache entries to remove when max size is exceeded. +When cache reaches `wttrin-cache-max-entries', remove the oldest 20% +to avoid frequent cleanup cycles. This value (0.20) means remove 1/5 +of entries, providing a reasonable buffer before the next cleanup.") + (defcustom wttrin-mode-line-favorite-location nil "Favorite location to display weather for in the mode-line. When nil, mode-line weather display is disabled. @@ -110,6 +116,13 @@ Default is 900 seconds (15 minutes)." :group 'wttrin :type 'integer) +(defcustom wttrin-mode-line-startup-delay 3 + "Seconds to delay initial mode-line weather fetch after Emacs starts. +This allows network stack and daemon initialization to complete before +fetching weather data. Recommended range: 1-5 seconds." + :group 'wttrin + :type 'integer) + (defcustom wttrin-mode-line-emoji-font "Noto Color Emoji" "Font family to use for mode-line weather emoji. Common color emoji fonts include: @@ -378,8 +391,8 @@ CALLBACK is called with the weather data string when ready, or nil on error." (push (cons k (car v)) entries)) wttrin--cache) (setq entries (sort entries (lambda (a b) (< (cdr a) (cdr b))))) - ;; Remove oldest 20% of entries - (dotimes (_ (/ (length entries) 5)) + ;; Remove oldest entries based on wttrin--cache-cleanup-percentage + (dotimes (_ (floor (* (length entries) wttrin--cache-cleanup-percentage))) (when entries (remhash (caar entries) wttrin--cache) (setq entries (cdr entries))))))) @@ -491,8 +504,8 @@ Force-refresh cache and update tooltip without opening buffer." wttrin-mode-line-favorite-location wttrin-mode-line-refresh-interval)) (when wttrin-mode-line-favorite-location - ;; Delay initial fetch by 3 seconds to allow network to initialize during startup - (run-at-time 3 nil #'wttrin--mode-line-fetch-weather) + ;; Delay initial fetch to allow network to initialize during startup + (run-at-time wttrin-mode-line-startup-delay nil #'wttrin--mode-line-fetch-weather) ;; Set up refresh timer (starts after the interval from now) (when wttrin--mode-line-timer (cancel-timer wttrin--mode-line-timer)) @@ -501,7 +514,8 @@ Force-refresh cache and update tooltip without opening buffer." wttrin-mode-line-refresh-interval #'wttrin--mode-line-fetch-weather)) (when (featurep 'wttrin-debug) - (message "wttrin mode-line: Initial fetch scheduled in 3 seconds, then every %s seconds" + (message "wttrin mode-line: Initial fetch scheduled in %s seconds, then every %s seconds" + wttrin-mode-line-startup-delay wttrin-mode-line-refresh-interval)))) (defun wttrin--mode-line-stop () -- cgit v1.2.3