Skip to content

Add dialect config validation with error handling#2428

Open
benfdking wants to merge 2 commits intomainfrom
claude/dialect-config-validation-aODAc
Open

Add dialect config validation with error handling#2428
benfdking wants to merge 2 commits intomainfrom
claude/dialect-config-validation-aODAc

Conversation

@benfdking
Copy link
Copy Markdown
Collaborator

Summary

This PR adds comprehensive validation and error handling for dialect configuration options. Dialect initialization functions now return Result<Dialect, String> instead of Option<Dialect>, enabling proper error reporting when configuration values are invalid.

Key Changes

  • Dialect config validation: Modified DialectConfig::from_value() to return Result<Self, String> instead of Self, allowing validation errors to be propagated
  • Type checking for config values: Added validation that known config keys have boolean-parseable values; returns descriptive errors for type mismatches (e.g., string or integer values for boolean options)
  • Unknown config key warnings: Implemented logging warnings for unrecognized configuration keys while still allowing dialect initialization to succeed
  • Updated all dialect functions: Changed all dialect(config: Option<&Value>) -> Dialect signatures to dialect(config: Option<&Value>) -> Result<Dialect, String> across all 13 dialect implementations
  • Updated kind_to_dialect(): Changed return type from Option<Dialect> to Result<Dialect, String> with descriptive error messages for disabled dialects
  • Comprehensive test coverage: Added 5 new tests validating invalid config values, superfluous config keys, and default behavior

Implementation Details

  • The dialect_config! macro now generates validation logic that:
    • Checks for unknown keys and logs warnings
    • Validates that known keys have boolean-compatible values
    • Returns errors with the problematic key name and expected type
  • All dialect initialization code updated to use .transpose()? pattern for proper error propagation
  • Updated all call sites throughout the codebase to handle the new Result return type
  • Added log workspace dependency for warning messages

https://claude.ai/code/session_01W8Ldw75CKMAj6dcjM6K3NV

claude and others added 2 commits March 16, 2026 14:38
Dialect config validation now errors when a known config option has
a non-boolean value (e.g. pg_trgm = "not_a_bool") and logs a warning
for unrecognized config keys. The DialectConfig::from_value trait
method and kind_to_dialect now return Result<_, String> to propagate
validation errors.

https://claude.ai/code/session_01W8Ldw75CKMAj6dcjM6K3NV
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2f3beebc92

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +137 to +138
let dialect = kind_to_dialect(&dialect_kind, dialect_config)
.expect("Failed to load dialect configuration");
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Handle dialect config errors without panicking

When a dialect option has an invalid type, kind_to_dialect() now returns Err, but FluffConfig::new() immediately calls .expect(...) here, which turns user config mistakes into a process panic. This affects normal entry points like from_file(), from_source(), and from_root(), so a malformed [sqruff:dialect:*] value can crash the CLI/library instead of surfacing a recoverable configuration error.

Useful? React with 👍 / 👎.

@github-actions
Copy link
Copy Markdown
Contributor

Benchmark for 3331b5c

Click to view benchmark
Test Base PR %
DepthMap::from_parent 52.4±0.92µs 52.7±0.92µs +0.57%
fix_complex_query 11.9±0.17ms 12.2±0.24ms +2.52%
fix_superlong 267.4±29.43ms 208.2±10.79ms -22.14%
parse_complex_query 4.2±0.05µs 4.1±0.06µs -2.38%
parse_expression_recursion 7.1±0.17µs 7.2±0.11µs +1.41%
parse_simple_query 1063.1±19.41ns 1063.8±14.18ns +0.07%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants