refactor: tighten the public API and name the placeholder-style test - #5
Merged
Conversation
Four things a read-through turned up. `Dimension` is no longer `#[non_exhaustive]`. An AIP List request has exactly these three query dimensions and the crate's scope excludes more, so a caller mapping them to RPC statuses should get a compile error if that ever changes rather than a `_` arm that absorbs it silently. Removing the attribute is backwards compatible; adding it back would not be, so before 0.1.0 is the moment. `Query` derives `Default`. Every field already had one, three of the four are absent on a first unfiltered page, and the derived default is fail-closed -- an empty column map rejects every path. The positional-dialect check moves into a named `is_positional` with the reasoning attached, including that it infers a behavioural property from rendered text and that the honest fix is a `Dialect::is_positional` in sqlx-cel. Writing that helper, I inverted the comparison -- `!=` classifies every numbered dialect as positional, which repeats binds that are already addressable and shifts the whole page. Seven tests caught it, which is the answer to whether the existing ones were load-bearing. The new one pins the case that motivated asking the dialect at all: SQLite's `?1` form is positional in syntax but addressable, and must not repeat.
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.
Follow-up to a read-through for idiomaticity.
clippy::all+pedanticwerealready clean, and
nursery+cargoturned up onlyredundant_pub_crateanddependency-version noise from sqlx's tree — so these came from reading, not
from lints.
Dimensionis no longer#[non_exhaustive]An AIP
Listrequest has exactly these three query dimensions, and the crate'sscope excludes taking on more. Marking the enum non-exhaustive forced every
downstream
match— mapping to a gRPC status, say — to carry a_arm that cannever fire, and denied them the compile error if a variant ever is added.
Removing the attribute is backwards compatible; adding it back would not be, so
before
0.1.0ships is the moment.QueryderivesDefaultEvery field already had a
Default, and three of the four are absent on a firstunfiltered page:
The default
Columnsmap is empty, which rejects every path — the deriveddefault is the fail-closed one, not an open door.
The placeholder-style check has a name
is_positional, with the reasoning attached: that it infers a behaviouralproperty from rendered text, that this is a smell, and that the honest fix is a
Dialect::is_positionalin sqlx-cel defaulting to exactly this comparison. I'llraise that upstream separately.
I also walked back a claim I made about the ordinal arithmetic in the positional
branch:
param_offset + repeated.len() - 1is not dead, it is the trueparameter ordinal of the bind. A positional dialect ignores the number, but it
is the honest answer, and the comment now says so instead of the code changing.
The bug this found
Writing the helper, I inverted the comparison.
!=classifies every numbereddialect as positional, which repeats binds that are already addressable and
shifts the entire page. Seven existing tests failed, which is a decent answer to
whether they were load-bearing.
The new test pins the case that motivated asking the dialect rather than
hardcoding the three built-ins: SQLite's
?1form is positional in syntax butstill addressable, so its values must not repeat. A "does the placeholder
contain
?" check would get that wrong.Testing
34 unit tests, 6 doctests, 6 SQLite round-trips, 6 Postgres round-trips; fmt,
clippy and
cargo doc -D warningsclean across the feature matrix.