Repository navigation
Stop optional TOML keys crashing, and name the key in config errors - #175
Merged
JordanMarr merged 2 commits intoOct 19, 2026
Merged
Conversation
michaelglass
changed the title
michaelglass
changed the title
michaelglass
changed the title
michaelglass
force-pushed
the
fix/toml-filters-optional-keys
branch
from
September 29, 2026 12:25
803ae15 to
4e1165a
Compare
michaelglass
marked this pull request as ready for review
September 29, 2026 12:27
A [filters] section with only one of include/exclude (or only restrictions) threw KeyNotFoundException for the missing key. So did a [sqlhydra_query_integration] section without provider_db_type_attributes. A missing include or exclude now reads as an empty list, which filters nothing. A missing provider_db_type_attributes now means true, the same as a missing section. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
michaelglass
force-pushed
the
fix/toml-filters-optional-keys
branch
from
September 29, 2026 13:12
4e1165a to
fe7ff7e
Compare
A missing required key threw a bare KeyNotFoundException, and a value of
the wrong type threw an InvalidCastException that did not say which key
was wrong.
Read the config through a Section, which knows its own name ("[general]",
"[filters.restrictions]") and replaces the Get and TryGet extensions on
TomlTable. When a required key is missing or a value has the wrong type,
the error names the section, the key and the TOML kinds, e.g.
"[general] key 'cli_mutable' should be a Boolean, but is a String."
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
michaelglass
force-pushed
the
fix/toml-filters-optional-keys
branch
from
September 29, 2026 13:28
fe7ff7e to
cd7d4c3
Compare
michaelglass
commented
Sep 29, 2026
| | _ -> failwith $"{name} key '{key}' should be {kind typeof<'T>}, but is {kind (value.GetType())}." | ||
|
|
||
| /// The value at `key`, or None when it is left out. Fails when the value has the wrong type. | ||
| member this.TryGet<'T>(key: string) : 'T option = |
Contributor
Author
There was a problem hiding this comment.
I like the functionality change but I'm generally not so happy with these names. Alas.
Merged
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.
Before
Leaving out an optional key crashed the CLI. A
[filters]section with onlyincludefailed:So did
[filters]with onlyexcludeor onlyrestrictions(the error namesinclude), and[sqlhydra_query_integration]withoutprovider_db_type_attributes.After
[filters]with onlyincludeKeyNotFoundException: The given key 'exclude' was not present in the dictionary.[filters]with onlyexcludeKeyNotFoundException: The given key 'include' was not present in the dictionary.[filters]with onlyrestrictionsKeyNotFoundException: The given key 'include' was not present in the dictionary.[sqlhydra_query_integration]withoutprovider_db_type_attributesKeyNotFoundException: The given key 'provider_db_type_attributes' was not present in the dictionary.connectionin[general]KeyNotFoundException: The given key 'connection' was not present in the dictionary.[general] is missing required key 'connection'.cli_mutable = "yes"InvalidCastException: Unable to cast object of type 'System.String' to type 'System.Boolean'.[general] key 'cli_mutable' should be a Boolean, but is a String.include = "dbo/*"InvalidCastException: Unable to cast object of type 'System.Char' to type 'System.String'.[filters] key 'include' should be an Array, but is a String.restrictions = { "Tables" = [ 1 ] }InvalidCastException: Unable to cast object of type 'System.Int64' to type 'System.String'.[filters.restrictions] key 'Tables' should be a String, but is an Integer.Over-engineering?
Could drop the second commit that improves error messages. you tell me.
Spikes, for context: we also tried reading the file into typed classes with Tomlyn (on 0.17, on Tomlyn 2.10.1). They didn't help: on 0.17, Tomlyn converts types loosely (
connection = trueloads as"True") and has no required keys, so it needed a reflection checker doing the same job as this PR'sSection.Castat more than twice the parser changes. On 2.10.1, Tomlyn checks types and required keys itself, but its errors don't name the key (Expected String token but was Boolean.), and fixing that means parsing its message text, on top of a two-major-version upgrade. This PR gets strict types and clear messages without either.