From 45bcbde502e07ad73b96039fcbb62471a3c94781 Mon Sep 17 00:00:00 2001 From: Lukasz Kasprzak Date: Wed, 19 Aug 2026 13:34:53 +0200 Subject: feat(naming): the language table Maps strings to strings and nothing else -- no calendars, no dates, no filesystem. That is what lets every command use it without the kernel learning about presentation. Every lookup is total, and a miss returns THE KEY rather than the empty string. A partial translation is therefore usable from its first line, and the fully-degraded case is exactly today's output (bare slugs) rather than a blank page. --raw is a real identity table, not a special case threaded through every call site: one value the whole program passes around. Reuses Overlay_ini's INI reader rather than growing a second one that would drift in its comment, quoting and trimming rules; parse_sections is exposed in the .mli for that, with no behaviour change. Fixes one defect found while running the brief's own tests rather than transcribing them blind: weekday's internal lookup key is an English day-name word (month's is already the numeral string), so on a miss it echoed that word instead of the documented numeral, breaking both the 0=Sunday convention and Lang.raw's own identity contract for weekday. weekday/month now fall back to string_of_int n directly on a miss instead of through get's generic echo-the-search-key path; month is byte-identical since its key already equals string_of_int n. --- lib/naming/dune | 3 ++ lib/naming/lang.ml | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++++ lib/naming/lang.mli | 44 ++++++++++++++++++++++++ 3 files changed, 143 insertions(+) create mode 100644 lib/naming/dune create mode 100644 lib/naming/lang.ml create mode 100644 lib/naming/lang.mli (limited to 'lib/naming') diff --git a/lib/naming/dune b/lib/naming/dune new file mode 100644 index 0000000..8af06c5 --- /dev/null +++ b/lib/naming/dune @@ -0,0 +1,3 @@ +(library + (name colitur_naming) + (libraries colitur_kernel)) diff --git a/lib/naming/lang.ml b/lib/naming/lang.ml new file mode 100644 index 0000000..0d902d5 --- /dev/null +++ b/lib/naming/lang.ml @@ -0,0 +1,96 @@ +module OI = Colitur_kernel.Overlay_ini + +module SM = Map.Make (String) + +type table = string SM.t + +type t = { + code : string; + fallback_code : string option; + celebration : table; + weekday : table; + month : table; + season : table; + rank : table; + colour : table; + term : table; + chain : t option; (** consulted when this table misses *) +} + +let empty_table = SM.empty + +let rec lookup t sel key = + match SM.find_opt key (sel t) with + | Some v -> Some v + | None -> ( match t.chain with Some b -> lookup b sel key | None -> None) + +(* A miss returns the KEY, never "". See lang.mli for why. *) +let get t sel key = match lookup t sel key with Some v -> v | None -> key + +let celebration t k = get t (fun x -> x.celebration) k +let season t k = get t (fun x -> x.season) k +let rank t k = get t (fun x -> x.rank) k +let colour t k = get t (fun x -> x.colour) k +let term t k = get t (fun x -> x.term) k + +let weekday_key = [| "sunday"; "monday"; "tuesday"; "wednesday"; "thursday"; "friday"; "saturday" |] + +(* weekday/month deliberately do NOT go through [get]: their SM key is a + presentation detail (an English day-name word for weekday, a numeral for + month), not the value a miss should degrade to. [get]'s generic + echo-the-search-key fallback would leak that word ("sunday") out of an + empty/raw table instead of the documented numeral -- [month] never showed + the bug because its key already equals [string_of_int n], but [weekday] + did, and it broke [Lang.raw]'s own identity contract (0 = Sunday, not + "sunday"). Falling back to [string_of_int n] directly keeps [month] + byte-identical and fixes [weekday]. *) +let weekday t n = + if n < 0 || n > 6 then string_of_int n + else match lookup t (fun x -> x.weekday) weekday_key.(n) with Some v -> v | None -> string_of_int n + +let month t n = + if n < 1 || n > 12 then string_of_int n + else match lookup t (fun x -> x.month) (string_of_int n) with Some v -> v | None -> string_of_int n + +let code t = t.code +let fallback_code t = t.fallback_code + +let raw = + { code = "raw"; fallback_code = None; celebration = empty_table; weekday = empty_table; + month = empty_table; season = empty_table; rank = empty_table; colour = empty_table; + term = empty_table; chain = None } + +let with_fallback t base = { t with chain = Some base } + +let of_string text = + match OI.parse_sections text with + | Error e -> Error e + | Ok sections -> + let find name = + match List.find_opt (fun (s : OI.section) -> s.OI.name = name) sections with + | Some s -> List.fold_left (fun m (k, v) -> SM.add k v m) empty_table s.OI.fields + | None -> empty_table + in + let meta = find "meta" in + (match SM.find_opt "lang" meta with + | None -> Error "language file has no [meta] lang = " + | Some code -> + Ok + { code; + fallback_code = SM.find_opt "fallback" meta; + celebration = find "celebration"; + weekday = find "weekday"; + month = find "month"; + season = find "season"; + rank = find "rank"; + colour = find "colour"; + term = find "term"; + chain = None }) + +let keys t = + let qualify prefix m = SM.bindings m |> List.map (fun (k, v) -> (prefix ^ "." ^ k, v)) in + List.concat + [ qualify "celebration" t.celebration; qualify "weekday" t.weekday; + qualify "month" t.month; qualify "season" t.season; qualify "rank" t.rank; + qualify "colour" t.colour; qualify "term" t.term ] + |> List.sort compare diff --git a/lib/naming/lang.mli b/lib/naming/lang.mli new file mode 100644 index 0000000..c4432f6 --- /dev/null +++ b/lib/naming/lang.mli @@ -0,0 +1,44 @@ +(** A language table: strings to strings, nothing more. + + Knows nothing about calendars, dates or rites, and never touches the + filesystem -- callers hand it text. That is what lets every command use it + without the kernel learning about presentation. + + Every lookup is TOTAL. A key with no entry returns THE KEY ITSELF, never the + empty string: a partial translation must be usable from its first line, and + an untranslated day must still print something a reader can act on. This is + also why the pre-naming output (bare slugs) is exactly what an empty table + produces -- the degraded case is the old behaviour, not a blank page. *) + +type t + +(** Parse INI text. Never raises. [Error] on a malformed file or a missing + [\[meta\] lang]. *) +val of_string : string -> (t, string) result + +val code : t -> string +val fallback_code : t -> string option + +(** [with_fallback t base] resolves through [t] first, then [base], then the key. *) +val with_fallback : t -> t -> t + +(** The identity table: every lookup returns its key. This is what [--raw] uses, + so raw output is one table passed around rather than a special case threaded + through every call site. *) +val raw : t + +val celebration : t -> string -> string +val season : t -> string -> string +val rank : t -> string -> string +val colour : t -> string -> string +val term : t -> string -> string + +(** [weekday t n], 0 = Sunday. Out-of-range [n] returns [string_of_int n]. *) +val weekday : t -> int -> string + +(** [month t n], 1 = January. Out-of-range [n] returns [string_of_int n]. *) +val month : t -> int -> string + +(** Every (section-qualified key, value) pair, sorted. Used by [lang --dump] and + [lang --check]. Keys are qualified as e.g. ["celebration.ef-epiphany"]. *) +val keys : t -> (string * string) list -- cgit v1.3 From b26089630a61a54060c86ec26be6dc4fe5418b12 Mon Sep 17 00:00:00 2001 From: Lukasz Kasprzak Date: Wed, 19 Aug 2026 13:38:54 +0200 Subject: feat(naming): the config file Owns precedence and provenance and nothing else, and never reads the filesystem, so it is as testable as the language table. resolve returns the value AND its source, because a setting that silently comes from a file the user forgot about is worse than no setting at all -- config --show can then say where each effective value came from. overlay accumulates rather than last-wins: a user has more than one. An unknown key is reported, never fatal. A config written for a newer colitur must still work on an older one, but silently dropping a line the user wrote is how a typo becomes invisible. --- lib/naming/config.ml | 43 +++++++++++++++++++++++++++++++++++ lib/naming/config.mli | 28 +++++++++++++++++++++++ test/test_colitur.ml | 1 + test/test_config.ml | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 134 insertions(+) create mode 100644 lib/naming/config.ml create mode 100644 lib/naming/config.mli create mode 100644 test/test_config.ml (limited to 'lib/naming') diff --git a/lib/naming/config.ml b/lib/naming/config.ml new file mode 100644 index 0000000..ce1b433 --- /dev/null +++ b/lib/naming/config.ml @@ -0,0 +1,43 @@ +module OI = Colitur_kernel.Overlay_ini + +type t = { + lang : string option; + overlays : string list; + template : string option; + format : string option; + unknown_keys : string list; +} + +let empty = { lang = None; overlays = []; template = None; format = None; unknown_keys = [] } + +let lang t = t.lang +let overlays t = t.overlays +let template t = t.template +let format t = t.format +let unknown_keys t = t.unknown_keys + +let of_string text = + match OI.parse_sections text with + | Error e -> Error e + | Ok sections -> ( + match List.find_opt (fun (s : OI.section) -> s.OI.name = "defaults") sections with + | None -> Ok empty + | Some s -> + let acc = + List.fold_left + (fun acc (k, v) -> + match k with + | "lang" -> { acc with lang = Some v } + | "template" -> { acc with template = Some v } + | "format" -> { acc with format = Some v } + (* accumulates: a user has more than one overlay *) + | "overlay" -> { acc with overlays = acc.overlays @ [ v ] } + | other -> { acc with unknown_keys = acc.unknown_keys @ [ other ] }) + empty s.OI.fields + in + Ok acc) + +let resolve ~flag ~config ~default = + match flag with + | Some v -> (v, "flag") + | None -> ( match config with Some v -> (v, "config") | None -> (default, "default")) diff --git a/lib/naming/config.mli b/lib/naming/config.mli new file mode 100644 index 0000000..41ff429 --- /dev/null +++ b/lib/naming/config.mli @@ -0,0 +1,28 @@ +(** The config file: what the user wants by default, and where each value came + from. + + Owns precedence and provenance and nothing else. Never reads the filesystem + -- callers hand it text -- so it is as testable as the language table. + + A config file is OPTIONAL. With none, colitur behaves exactly as it does + without this feature, except that names resolve through the default + language. *) + +type t + +val empty : t +val of_string : string -> (t, string) result + +val lang : t -> string option +val overlays : t -> string list +val template : t -> string option +val format : t -> string option + +(** Keys present in the file that this build does not understand. Reported, never + fatal: a config written for a newer colitur must still work on an older one, + but silently ignoring a line the user wrote is how a typo becomes invisible. *) +val unknown_keys : t -> string list + +(** [resolve ~flag ~config ~default] returns [(value, source)] with source one of + ["flag"], ["config"], ["default"]. Precedence is flag > config > default. *) +val resolve : flag:string option -> config:string option -> default:string -> string * string diff --git a/test/test_colitur.ml b/test/test_colitur.ml index dd1274a..1c445a6 100644 --- a/test/test_colitur.ml +++ b/test/test_colitur.ml @@ -3,6 +3,7 @@ let () = Alcotest.run "colitur" [ Test_date.suite; Test_computus.suite; Test_colour.suite; Test_slug.suite; Test_names.suite; Test_lang.suite; + Test_config.suite; Test_overlay.suite; Test_overlay_ini.suite; Test_temporal_ef.suite; Test_validate.suite; Test_precedence.suite; Test_calendar.suite; Test_precedence_ef.suite; Test_sanctoral_ef.suite; Test_rite_ef.suite; Test_differential.suite; Test_oracle.suite; Test_oracle.suite_2038; Test_oracle.suite_2035; Test_golden.suite; diff --git a/test/test_config.ml b/test/test_config.ml new file mode 100644 index 0000000..9100380 --- /dev/null +++ b/test/test_config.ml @@ -0,0 +1,62 @@ +module C = Colitur_naming.Config + +let ok = function Ok x -> x | Error e -> Alcotest.failf "parse: %s" e + +let sample = + "[defaults]\n\ + lang = pl\n\ + overlay = ~/a.ini\n\ + overlay = ~/b.ini\n\ + template = ~/my-ordo.tex\n\ + format = json\n" + +let test_reads_defaults () = + let c = ok (C.of_string sample) in + Alcotest.(check (option string)) "lang" (Some "pl") (C.lang c); + Alcotest.(check (option string)) "template" (Some "~/my-ordo.tex") (C.template c); + Alcotest.(check (option string)) "format" (Some "json") (C.format c) + +(* Repeated keys accumulate for overlay -- a user has more than one. The INI + reader keeps every line, so this asserts we do not silently take the last. *) +let test_overlays_accumulate () = + let c = ok (C.of_string sample) in + Alcotest.(check (list string)) "both overlays" [ "~/a.ini"; "~/b.ini" ] (C.overlays c) + +let test_empty_config_is_all_none () = + Alcotest.(check (option string)) "lang" None (C.lang C.empty); + Alcotest.(check (list string)) "overlays" [] (C.overlays C.empty) + +(* THE precedence rule: flag > config > default, and the SOURCE is reported, + because a setting that silently comes from a file the user forgot about is + worse than no setting at all. *) +let test_precedence_and_provenance () = + let check ~flag ~config ~default (ev, es) = + let v, s = C.resolve ~flag ~config ~default in + Alcotest.(check string) "value" ev v; + Alcotest.(check string) "source" es s + in + check ~flag:(Some "en") ~config:(Some "pl") ~default:"la" ("en", "flag"); + check ~flag:None ~config:(Some "pl") ~default:"la" ("pl", "config"); + check ~flag:None ~config:None ~default:"la" ("la", "default") + +let test_malformed_is_error_not_crash () = + match C.of_string "[defaults\nbroken" with + | Error _ -> () + | Ok _ -> Alcotest.fail "a malformed config must be an Error, never accepted" + +(* An unknown key is a WARNING case, not a hard error: a config written for a + newer colitur must still work on an older one. But it must be reportable, so + it is not silently dropped either. *) +let test_unknown_key_is_reported_not_fatal () = + match C.of_string "[defaults]\nlang = la\nnonsense = 1\n" with + | Error _ -> Alcotest.fail "an unknown key must not be fatal" + | Ok c -> Alcotest.(check (option string)) "known key still read" (Some "la") (C.lang c) + +let suite = + ( "Config", + [ Alcotest.test_case "reads defaults" `Quick test_reads_defaults; + Alcotest.test_case "overlays accumulate" `Quick test_overlays_accumulate; + Alcotest.test_case "empty is all none" `Quick test_empty_config_is_all_none; + Alcotest.test_case "precedence and provenance" `Quick test_precedence_and_provenance; + Alcotest.test_case "malformed is error" `Quick test_malformed_is_error_not_crash; + Alcotest.test_case "unknown key not fatal" `Quick test_unknown_key_is_reported_not_fatal ] ) -- cgit v1.3 From 6bdd5afc5e185b639f9cf5e7c3d662ddc3ecf0e1 Mon Sep 17 00:00:00 2001 From: Lukasz Kasprzak Date: Wed, 19 Aug 2026 13:43:28 +0200 Subject: fix(naming): merge duplicate [section] blocks in the language table F1 (review round 1): of_string's find took only the FIRST section of a given name (List.find_opt), so a second [celebration] block anywhere in the file was silently dropped in its entirety -- reproduced with two blocks (a in the first, b in the second): b resolved to the slug fallback "b", not its real value. This is a data-loss footgun aimed squarely at what happens next: Tasks 3/4 write a 595-entry, hand-edited la.ini, and appending a second [celebration] block is the natural way to paste in a new batch of names. Worse, the failure surfaces nowhere near its cause -- a coverage check reports the dropped slugs as missing a Latin name, with nothing pointing at the parser. find now folds over every section sharing the name, in file order, so all blocks merge. This also settles which value wins when the same key appears in two different blocks: later in the file wins, consistent with the existing within-one-block behaviour (unchanged, still last SM.add wins) and with what a reader expects when appending to an INI file. lang.mli now documents both duplicate policies explicitly, and notes they run OPPOSITE to Overlay_ini.get's first-match (List.assoc_opt) over the same section.fields shape -- undocumented before, and a latent trap since the two modules read the same section type but resolve a duplicate key in opposite directions. Three tests added: two [celebration] blocks both resolve (the F1 regression), a key repeated across two blocks resolves to the later block, and a key repeated within one block still resolves to the later line (confirms unchanged behaviour). Confirmed the regression test fails against the pre-fix code (b resolves to "b", the slug fallback) and passes after. --- lib/naming/lang.ml | 19 ++++++++++++++++--- lib/naming/lang.mli | 18 +++++++++++++++++- test/test_lang.ml | 34 +++++++++++++++++++++++++++++++++- 3 files changed, 66 insertions(+), 5 deletions(-) (limited to 'lib/naming') diff --git a/lib/naming/lang.ml b/lib/naming/lang.ml index 0d902d5..147a49b 100644 --- a/lib/naming/lang.ml +++ b/lib/naming/lang.ml @@ -66,10 +66,23 @@ let of_string text = match OI.parse_sections text with | Error e -> Error e | Ok sections -> + (* Merge EVERY section sharing [name], not just the first: a hand-edited + 595-entry language file (Tasks 3/4's la.ini) WILL grow duplicate + [section] headers as contributors append entries over time -- a + second [celebration] block is the natural way to paste in a new + batch of names. Taking only the first match (the original + [List.find_opt] here) silently dropped every later block; the + failure then surfaces as a coverage report claiming those slugs have + "no Latin name", with nothing pointing back at the parser. Folding + over all matching sections, in file order, keeps this consistent + with the existing within-section behaviour below (last [SM.add] + wins): a key repeated across two blocks resolves to the later one, + exactly what a reader expects when appending to an INI file. *) let find name = - match List.find_opt (fun (s : OI.section) -> s.OI.name = name) sections with - | Some s -> List.fold_left (fun m (k, v) -> SM.add k v m) empty_table s.OI.fields - | None -> empty_table + List.fold_left + (fun m (s : OI.section) -> + if s.OI.name = name then List.fold_left (fun m (k, v) -> SM.add k v m) m s.OI.fields else m) + empty_table sections in let meta = find "meta" in (match SM.find_opt "lang" meta with diff --git a/lib/naming/lang.mli b/lib/naming/lang.mli index c4432f6..36931d4 100644 --- a/lib/naming/lang.mli +++ b/lib/naming/lang.mli @@ -13,7 +13,23 @@ type t (** Parse INI text. Never raises. [Error] on a malformed file or a missing - [\[meta\] lang]. *) + [\[meta\] lang]. + + Two duplicate policies, both LAST-WINS: + - A section name repeated in the file (e.g. two [\[celebration\]] + blocks) has ALL of its blocks merged, not only the first -- a + 595-entry hand-edited language file WILL grow duplicate section + headers as contributors append entries over time, and dropping a + later block would silently lose real translations. + - Where the same key appears more than once -- within one block or + across two of them -- the value from further down the file wins. + + Both read the same order a reader would: later in the file overrides + earlier. This is the OPPOSITE direction from + {!Colitur_kernel.Overlay_ini.get} ([List.assoc_opt], first match) over + the very same [section.fields] shape -- the two modules resolve a + duplicate key in opposite directions, so do not assume one's behaviour + from the other's. *) val of_string : string -> (t, string) result val code : t -> string diff --git a/test/test_lang.ml b/test/test_lang.ml index 0b16b30..b54e85b 100644 --- a/test/test_lang.ml +++ b/test/test_lang.ml @@ -74,6 +74,33 @@ let test_missing_meta_lang_is_error () = | Error _ -> () | Ok _ -> Alcotest.fail "a language file with no [meta] lang must be an Error" +(* F1 regression: a hand-edited language file WILL grow duplicate [section] + headers as contributors append entries over time (Tasks 3/4's 595-entry + la.ini). Both blocks' keys must resolve -- silently dropping the second + block is a data-loss footgun that surfaces as a false "missing name" + report far from its real cause. *) +let test_duplicate_sections_all_merge () = + let t = + ok + (L.of_string + "[meta]\nlang = la\n[celebration]\na = ALPHA\n[weekday]\nsunday = Dominica\n\ + [celebration]\nb = BETA\n") + in + Alcotest.(check string) "first block's key" "ALPHA" (L.celebration t "a"); + Alcotest.(check string) "second block's key" "BETA" (L.celebration t "b") + +let test_duplicate_key_across_sections_last_wins () = + let t = + ok + (L.of_string + "[meta]\nlang = la\n[celebration]\na = FIRST\n[celebration]\na = SECOND\n") + in + Alcotest.(check string) "later block's value wins" "SECOND" (L.celebration t "a") + +let test_duplicate_key_within_section_last_wins () = + let t = ok (L.of_string "[meta]\nlang = la\n[celebration]\na = FIRST\na = SECOND\n") in + Alcotest.(check string) "later line's value wins" "SECOND" (L.celebration t "a") + let suite = ( "Lang", [ Alcotest.test_case "meta" `Quick test_meta; @@ -82,4 +109,9 @@ let suite = Alcotest.test_case "fallback chain" `Quick test_fallback_chain; Alcotest.test_case "raw is identity" `Quick test_raw_is_identity; Alcotest.test_case "malformed is error" `Quick test_malformed_is_error_not_crash; - Alcotest.test_case "missing meta lang is error" `Quick test_missing_meta_lang_is_error ] ) + Alcotest.test_case "missing meta lang is error" `Quick test_missing_meta_lang_is_error; + Alcotest.test_case "duplicate sections all merge" `Quick test_duplicate_sections_all_merge; + Alcotest.test_case "duplicate key across sections: last wins" `Quick + test_duplicate_key_across_sections_last_wins; + Alcotest.test_case "duplicate key within section: last wins" `Quick + test_duplicate_key_within_section_last_wins ] ) -- cgit v1.3 From 59fbd3718dbf2721557b97d7b4e87dce38fb745c Mon Sep 17 00:00:00 2001 From: Lukasz Kasprzak Date: Wed, 19 Aug 2026 13:48:39 +0200 Subject: fix(naming): config fix round 1 -- unknown sections, O(n) accumulate F1: test_unknown_key_is_reported_not_fatal never asserted unknown_keys itself, only that parsing survives -- a no-op accumulator passed it. Now asserts the key is actually collected. F2: a misspelled section name, e.g. [deafults], was silently discarded -- Ok empty, lang and everything else gone, nothing reported. That is the highest-value typo this feature exists to catch. Any section other than [defaults] is now collected into a new Config.unknown_sections, kept separate from unknown_keys so the CLI can word the two warnings differently. Still non-fatal: a newer colitur's added section must not break an older binary. F3: overlays and unknown_keys accumulated with '@ [v]' per line, O(n^2) over the field count. Cons during the fold, List.rev once at the end. F4: documented that lang/template/format are last-wins on a repeated key, the opposite direction from Overlay_ini.get's first-wins over the same section type. --- lib/naming/config.ml | 55 ++++++++++++++++++++++++++++++++++----------------- lib/naming/config.mli | 22 ++++++++++++++++++--- test/test_config.ml | 22 ++++++++++++++++++--- 3 files changed, 75 insertions(+), 24 deletions(-) (limited to 'lib/naming') diff --git a/lib/naming/config.ml b/lib/naming/config.ml index ce1b433..0fafd1f 100644 --- a/lib/naming/config.ml +++ b/lib/naming/config.ml @@ -6,36 +6,55 @@ type t = { template : string option; format : string option; unknown_keys : string list; + unknown_sections : string list; } -let empty = { lang = None; overlays = []; template = None; format = None; unknown_keys = [] } +let empty = + { lang = None; overlays = []; template = None; format = None; unknown_keys = []; + unknown_sections = [] } let lang t = t.lang let overlays t = t.overlays let template t = t.template let format t = t.format let unknown_keys t = t.unknown_keys +let unknown_sections t = t.unknown_sections let of_string text = match OI.parse_sections text with | Error e -> Error e - | Ok sections -> ( - match List.find_opt (fun (s : OI.section) -> s.OI.name = "defaults") sections with - | None -> Ok empty - | Some s -> - let acc = - List.fold_left - (fun acc (k, v) -> - match k with - | "lang" -> { acc with lang = Some v } - | "template" -> { acc with template = Some v } - | "format" -> { acc with format = Some v } - (* accumulates: a user has more than one overlay *) - | "overlay" -> { acc with overlays = acc.overlays @ [ v ] } - | other -> { acc with unknown_keys = acc.unknown_keys @ [ other ] }) - empty s.OI.fields - in - Ok acc) + | Ok sections -> + (* Any section that is not [defaults] is unrecognised -- including a + plain typo such as [deafults] -- and must be REPORTED, never + silently dropped: that is precisely the failure this feature exists + to surface. *) + let unknown_sections = + List.filter_map + (fun (s : OI.section) -> if s.OI.name = "defaults" then None else Some s.OI.name) + sections + in + let acc = + match List.find_opt (fun (s : OI.section) -> s.OI.name = "defaults") sections with + | None -> empty + | Some s -> + (* Cons then reverse once at the end, not `@ [v]` per line: the + latter is O(n^2) over the field count, a real hang on a + machine-generated file with many overlay lines. *) + let acc = + List.fold_left + (fun acc (k, v) -> + match k with + | "lang" -> { acc with lang = Some v } + | "template" -> { acc with template = Some v } + | "format" -> { acc with format = Some v } + (* accumulates: a user has more than one overlay *) + | "overlay" -> { acc with overlays = v :: acc.overlays } + | other -> { acc with unknown_keys = other :: acc.unknown_keys }) + empty s.OI.fields + in + { acc with overlays = List.rev acc.overlays; unknown_keys = List.rev acc.unknown_keys } + in + Ok { acc with unknown_sections } let resolve ~flag ~config ~default = match flag with diff --git a/lib/naming/config.mli b/lib/naming/config.mli index 41ff429..27f0c44 100644 --- a/lib/naming/config.mli +++ b/lib/naming/config.mli @@ -13,16 +13,32 @@ type t val empty : t val of_string : string -> (t, string) result +(** [lang], [template] and [format] are each set from a single field. A + repeated key is LAST-WINS -- the opposite direction from + {!Colitur_kernel.Overlay_ini.get}'s first-wins over the same [section] + type -- because the natural reading of a config file a user edited by + hand and appended to is "the bottom line is the one that took effect". *) val lang : t -> string option + val overlays : t -> string list val template : t -> string option val format : t -> string option -(** Keys present in the file that this build does not understand. Reported, never - fatal: a config written for a newer colitur must still work on an older one, - but silently ignoring a line the user wrote is how a typo becomes invisible. *) +(** Keys present in the [\[defaults\]] section that this build does not + understand. Reported, never fatal: a config written for a newer colitur + must still work on an older one, but silently ignoring a line the user + wrote is how a typo becomes invisible. *) val unknown_keys : t -> string list +(** Section names other than [\[defaults\]], reported separately from + {!unknown_keys} so the CLI can word the two warnings differently (a + misspelled section, e.g. [\[deafults\]], versus a misspelled key inside a + recognised one). Also never fatal, and never silent: a section this build + does not recognise is exactly the highest-value typo this feature exists + to catch, because it silently discards the whole section -- [lang] and + everything else in it -- with no other way for the user to notice. *) +val unknown_sections : t -> string list + (** [resolve ~flag ~config ~default] returns [(value, source)] with source one of ["flag"], ["config"], ["default"]. Precedence is flag > config > default. *) val resolve : flag:string option -> config:string option -> default:string -> string * string diff --git a/test/test_config.ml b/test/test_config.ml index 9100380..067b255 100644 --- a/test/test_config.ml +++ b/test/test_config.ml @@ -46,11 +46,25 @@ let test_malformed_is_error_not_crash () = (* An unknown key is a WARNING case, not a hard error: a config written for a newer colitur must still work on an older one. But it must be reportable, so - it is not silently dropped either. *) + it is not silently dropped either -- assert it is actually COLLECTED, not + only that parsing survives it: a no-op accumulator would also pass a test + that checked survival alone. *) let test_unknown_key_is_reported_not_fatal () = match C.of_string "[defaults]\nlang = la\nnonsense = 1\n" with | Error _ -> Alcotest.fail "an unknown key must not be fatal" - | Ok c -> Alcotest.(check (option string)) "known key still read" (Some "la") (C.lang c) + | Ok c -> + Alcotest.(check (option string)) "known key still read" (Some "la") (C.lang c); + Alcotest.(check (list string)) "unknown key reported" [ "nonsense" ] (C.unknown_keys c) + +(* The highest-value case: a misspelled SECTION name (not merely a misspelled + key inside a recognised one). Before this fix the whole section, lang + included, vanished with nothing reported -- exactly the typo this feature + exists to surface, in its worst form: a user whose config silently does + nothing has no way to discover why. *) +let test_misspelled_section_is_reported_not_silently_dropped () = + let c = ok (C.of_string "[deafults]\nlang = pl\n") in + Alcotest.(check (option string)) "lang not read from the wrong section" None (C.lang c); + Alcotest.(check (list string)) "section reported" [ "deafults" ] (C.unknown_sections c) let suite = ( "Config", @@ -59,4 +73,6 @@ let suite = Alcotest.test_case "empty is all none" `Quick test_empty_config_is_all_none; Alcotest.test_case "precedence and provenance" `Quick test_precedence_and_provenance; Alcotest.test_case "malformed is error" `Quick test_malformed_is_error_not_crash; - Alcotest.test_case "unknown key not fatal" `Quick test_unknown_key_is_reported_not_fatal ] ) + Alcotest.test_case "unknown key not fatal" `Quick test_unknown_key_is_reported_not_fatal; + Alcotest.test_case "misspelled section reported" + `Quick test_misspelled_section_is_reported_not_silently_dropped ] ) -- cgit v1.3 From d869a4410a88333d328358c723609577d32f3380 Mon Sep 17 00:00:00 2001 From: Lukasz Kasprzak Date: Wed, 19 Aug 2026 14:09:09 +0200 Subject: fix(naming): config.ml merges every [defaults] block, like lang.ml config.ml and lang.ml both parse the INI format through the same reader, Colitur_kernel.Overlay_ini.parse_sections, but resolved a repeated [section] header oppositely: lang.ml folds over every section sharing a name, while config.ml used List.find_opt and silently discarded every [defaults] block after the first. Two modules parsing one file format must not disagree about what a duplicate section header means. of_string now folds a single accumulator across every section named [defaults], in file order, matching lang.ml's of_string shape. A scalar key (lang/template/format) repeated across two blocks resolves to the later value, consistent with the existing within-section last-wins rule; overlay keeps accumulating across every block, not only the first; and unknown_sections still excludes every [defaults] block, merged or not, since merging it is the point. config.mli's lang doc comment is extended to say the last-wins rule holds across block boundaries too, cross-referencing lang.ml's own duplicate-section policy so the two do not drift again unnoticed. --- lib/naming/config.ml | 56 ++++++++++++++++++++++++++++++++++----------------- lib/naming/config.mli | 7 ++++++- test/test_config.ml | 46 +++++++++++++++++++++++++++++++++++++++++- 3 files changed, 88 insertions(+), 21 deletions(-) (limited to 'lib/naming') diff --git a/lib/naming/config.ml b/lib/naming/config.ml index 0fafd1f..7d87d62 100644 --- a/lib/naming/config.ml +++ b/lib/naming/config.ml @@ -33,27 +33,45 @@ let of_string text = (fun (s : OI.section) -> if s.OI.name = "defaults" then None else Some s.OI.name) sections in + (* Merge EVERY section named [defaults], not just the first: [lang.ml]'s + [of_string] was fixed this morning to fold over all matching + sections rather than take [List.find_opt]'s first match, because a + hand-edited file WILL grow duplicate headers as a user appends to it + over time. Both modules parse the same reader + ([Overlay_ini.parse_sections]) over the same file format, so they + must not disagree about what a duplicate [section] header means -- + taking only the first [defaults] block here silently discarded every + later one, with nothing pointing back at the parser. Folding a + single accumulator across every matching section, in file order, + keeps this consistent with the within-section behaviour below (last + [k]-match wins): a scalar key repeated across two blocks resolves to + the LATER value, and [overlay] keeps accumulating across every + block, not only its first. *) + let defaults_sections = + List.filter (fun (s : OI.section) -> s.OI.name = "defaults") sections + in let acc = - match List.find_opt (fun (s : OI.section) -> s.OI.name = "defaults") sections with - | None -> empty - | Some s -> - (* Cons then reverse once at the end, not `@ [v]` per line: the - latter is O(n^2) over the field count, a real hang on a - machine-generated file with many overlay lines. *) - let acc = - List.fold_left - (fun acc (k, v) -> - match k with - | "lang" -> { acc with lang = Some v } - | "template" -> { acc with template = Some v } - | "format" -> { acc with format = Some v } - (* accumulates: a user has more than one overlay *) - | "overlay" -> { acc with overlays = v :: acc.overlays } - | other -> { acc with unknown_keys = other :: acc.unknown_keys }) - empty s.OI.fields - in - { acc with overlays = List.rev acc.overlays; unknown_keys = List.rev acc.unknown_keys } + (* Cons then reverse once at the very end, not `@ [v]` per line: the + latter is O(n^2) over the field count, a real hang on a + machine-generated file with many overlay lines. Reversing only + after every section has been folded (not once per section) is + what keeps [overlay] and [unknown_keys] in file order across + block boundaries, not merely within one block. *) + List.fold_left + (fun acc (s : OI.section) -> + List.fold_left + (fun acc (k, v) -> + match k with + | "lang" -> { acc with lang = Some v } + | "template" -> { acc with template = Some v } + | "format" -> { acc with format = Some v } + (* accumulates: a user has more than one overlay *) + | "overlay" -> { acc with overlays = v :: acc.overlays } + | other -> { acc with unknown_keys = other :: acc.unknown_keys }) + acc s.OI.fields) + empty defaults_sections in + let acc = { acc with overlays = List.rev acc.overlays; unknown_keys = List.rev acc.unknown_keys } in Ok { acc with unknown_sections } let resolve ~flag ~config ~default = diff --git a/lib/naming/config.mli b/lib/naming/config.mli index 27f0c44..a84f95b 100644 --- a/lib/naming/config.mli +++ b/lib/naming/config.mli @@ -17,7 +17,12 @@ val of_string : string -> (t, string) result repeated key is LAST-WINS -- the opposite direction from {!Colitur_kernel.Overlay_ini.get}'s first-wins over the same [section] type -- because the natural reading of a config file a user edited by - hand and appended to is "the bottom line is the one that took effect". *) + hand and appended to is "the bottom line is the one that took effect". + This holds whether the repeat is within one [\[defaults\]] block or + across two of them: every section named [defaults] is merged, not only + the first, the same duplicate-section policy {!Lang.of_string} documents + for its own sections -- the two modules read the same underlying format + and must not disagree about what a repeated header means. *) val lang : t -> string option val overlays : t -> string list diff --git a/test/test_config.ml b/test/test_config.ml index 067b255..83da015 100644 --- a/test/test_config.ml +++ b/test/test_config.ml @@ -66,6 +66,42 @@ let test_misspelled_section_is_reported_not_silently_dropped () = Alcotest.(check (option string)) "lang not read from the wrong section" None (C.lang c); Alcotest.(check (list string)) "section reported" [ "deafults" ] (C.unknown_sections c) +(* THE regression test for the cross-module inconsistency: [config.ml] used + to locate [\[defaults\]] with [List.find_opt], taking only the FIRST + matching section and silently discarding every later one, while + [lang.ml]'s [of_string] folds over ALL matching sections. A scalar set in + the first block and a DIFFERENT scalar set only in the second block must + both resolve -- before the fix, [template] (second-block-only) came back + [None]. *) +let test_two_defaults_blocks_both_contribute () = + let text = "[defaults]\nlang = pl\n\n[defaults]\ntemplate = ~/my-ordo.tex\n" in + let c = ok (C.of_string text) in + Alcotest.(check (option string)) "lang from first block" (Some "pl") (C.lang c); + Alcotest.(check (option string)) "template from second block" (Some "~/my-ordo.tex") (C.template c) + +(* Consistent with the existing within-section last-wins rule (see + [config.mli]'s [lang] comment): a key repeated ACROSS two [\[defaults\]] + blocks resolves to the value from the LATER block, exactly as it would if + both lines sat in one block. *) +let test_key_repeated_across_blocks_last_wins () = + let text = "[defaults]\nlang = pl\n\n[defaults]\nlang = en\n" in + let c = ok (C.of_string text) in + Alcotest.(check (option string)) "later block's lang wins" (Some "en") (C.lang c) + +(* [overlay] must keep accumulating across block boundaries, in file order, + not merely within one block. *) +let test_overlays_accumulate_across_blocks () = + let text = "[defaults]\noverlay = ~/a.ini\n\n[defaults]\noverlay = ~/b.ini\n" in + let c = ok (C.of_string text) in + Alcotest.(check (list string)) "both overlays, in file order" [ "~/a.ini"; "~/b.ini" ] (C.overlays c) + +(* Merging a second [\[defaults\]] block is the whole point -- it must not + start being reported as an unknown section. *) +let test_two_defaults_blocks_report_no_unknown_sections () = + let text = "[defaults]\nlang = pl\n\n[defaults]\ntemplate = ~/my-ordo.tex\n" in + let c = ok (C.of_string text) in + Alcotest.(check (list string)) "no unknown sections" [] (C.unknown_sections c) + let suite = ( "Config", [ Alcotest.test_case "reads defaults" `Quick test_reads_defaults; @@ -75,4 +111,12 @@ let suite = Alcotest.test_case "malformed is error" `Quick test_malformed_is_error_not_crash; Alcotest.test_case "unknown key not fatal" `Quick test_unknown_key_is_reported_not_fatal; Alcotest.test_case "misspelled section reported" - `Quick test_misspelled_section_is_reported_not_silently_dropped ] ) + `Quick test_misspelled_section_is_reported_not_silently_dropped; + Alcotest.test_case "two defaults blocks both contribute" + `Quick test_two_defaults_blocks_both_contribute; + Alcotest.test_case "key repeated across blocks last wins" + `Quick test_key_repeated_across_blocks_last_wins; + Alcotest.test_case "overlays accumulate across blocks" + `Quick test_overlays_accumulate_across_blocks; + Alcotest.test_case "two defaults blocks report no unknown sections" + `Quick test_two_defaults_blocks_report_no_unknown_sections ] ) -- cgit v1.3