Skip to content

refactor: ask the dialect whether it is positional - #6

Merged
iamralch merged 1 commit into
mainfrom
refactor/use-dialect-is-positional
Sep 5, 2026
Merged

refactor: ask the dialect whether it is positional#6
iamralch merged 1 commit into
mainfrom
refactor/use-dialect-is-positional

Conversation

@iamralch

@iamralch iamralch commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Follow-up to sqlx-contrib/sqlx-cel#4, which added Dialect::is_positional.

The local inference in cursor.rs goes away — the trait answers it now:

-let positional = is_positional(dialect);
+let positional = dialect.is_positional();

The comparison the local helper made is the trait's default, so nothing in the
test suite changes. That includes the custom ?1 dialect in cursor.rs's tests,
which does not override the method and therefore exercises upstream's default
and gets the same answer the local copy gave — a useful cross-check that the
move was behaviour-preserving rather than merely compiling.

The reasoning moved upstream with the code, so the doc comment here just points
at Dialect::is_positional instead of restating it and apologising for
inferring a behavioural property from rendered text.

Testing

34 unit tests, 6 doctests, 6 SQLite round-trips, 6 Postgres round-trips; fmt,
clippy and cargo doc -D warnings clean.

sqlx-cel now answers this itself, so the local inference goes. The
comparison it made is the trait's default, which is why nothing in the
test suite changes -- including the custom `?1` dialect, which does not
override the method and so exercises that default and gets the same
answer the local copy gave.

The reasoning moves upstream with the code: `Dialect::is_positional`
documents why the property matters and what the middle case is.
@iamralch
iamralch merged commit 4c39816 into main Sep 5, 2026
4 checks passed
@iamralch
iamralch deleted the refactor/use-dialect-is-positional branch September 5, 2026 11:23
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.

1 participant