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.mli | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 lib/naming/config.mli (limited to 'lib/naming/config.mli') 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 -- 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/config.mli') 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/config.mli') 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