fix: migrate env-var substitution to serde-saphyr properties - #2868
Closed
CommanderStorm wants to merge 3 commits into
Closed
fix: migrate env-var substitution to serde-saphyr properties#2868CommanderStorm wants to merge 3 commits into
CommanderStorm wants to merge 3 commits into
Conversation
…e#2851) Replaces `subst::substitute`'s text-level preprocessing — which scanned comments and quoted strings — with `serde-saphyr`'s `properties` feature that only interpolates inside plain YAML scalar values. Adopts `PropertySyntax::BracedOrBare` so existing configs using `${VAR}`, `${VAR:-default}`, and bare `$VAR` keep working unchanged. Drops `subst` from the workspace. Pins `serde-saphyr` to the master rev containing PRs maplibre#122/maplibre#125/maplibre#128/maplibre#129 (defaults, modifiers, error-on-missing forms, bare syntax) until 0.0.28 is published; tracked in deny.toml. Substitution failures now flow through `ConfigFileError::YamlParseError` with a wrapper that emits `martin::config::substitution` as the diagnostic code (preserved from the old dedicated variant) when the underlying saphyr error is an `UnresolvedProperty` / `InvalidPropertyName` / `PropertyRequiredButUnset` / `PropertyRequiredButEmpty`. Updated inline insta snapshots across the failing-deserializer tests so all YAML errors render consistent code/help/url footers.
Clippy's `doc_markdown` lint (enabled via workspace pedantic) requires URLs in doc comments to be enclosed in angle brackets or rendered as Markdown links.
Member
Author
Update — integration tests surfaced a subst-specific syntax gapThree CI groups are red. Two have the same root cause; one is the cargo-publish dry-run which is expected.
|
Member
Author
|
ups, left claude unatented for a bit 👀 |
Performance Comparison
|
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.
Fixes #2851.
Summary
subst::substitute's text-level preprocessing — which scanned comments and quoted strings — withserde-saphyr'spropertiesfeature, which only interpolates inside plain YAML scalar values. Comments containing${VAR}no longer abort startup.PropertySyntax::BracedOrBareso existing configs using${VAR},${VAR:-default}, and bare$VARkeep working unchanged.substfrom the workspace. Pinsserde-saphyrto the master rev261ccbe6…containing the four PRs we contributed upstream (chore: update dependencies #122 defaults, chore(deps): bump serde_json from 1.0.57 to 1.0.58 #125 modifiers, chore(deps): bump actix-cors from 0.2.0 to 0.4.1 #128 error-on-missing, chore(deps): bump serde_json from 1.0.58 to 1.0.59 #129 bare syntax). Allowed viadeny.toml; both haveTODOs to drop once 0.0.28 is published.ConfigFileError::YamlParseErrorwith a wrapper that emitsmartin::config::substitutionas the diagnosticcode(preserved from the old dedicated variant) when the underlying saphyr error is property-related, otherwisemartin::config::yaml. Inline insta snapshots across the failing-deserializer tests were updated to match the consistent code/help/url footer.Behavior change to flag in release notes: substitution no longer happens inside quoted or block scalars. A YAML like
connection_string: \"\${DATABASE_URL}\"now keeps the literal\${DATABASE_URL}instead of expanding. Drop the quotes (or useconnection_string: \${DATABASE_URL}) to get the previous behavior.Test plan
cargo test -p martin --lib --no-default-features --features postgres,pmtiles— 212/212 passsubstitution_ignores_dollar_tokens_in_commentsusing the exact YAML from Martin tries to substitute environment variables in config comments #2851cargo fmt --all --checkcleanallow-gitentry is the load-bearing change)-D warningsworkspace-wide