Skip to content

chore: fix remaining implicit Optional type hints, enforce with RUF013 - #1343

Merged
thomasht86 merged 3 commits into
masterfrom
fix/remaining-implicit-optionals
Sep 1, 2026
Merged

thomasht86 merged 3 commits into
masterfrom
fix/remaining-implicit-optionals

Conversation

@thomasht86

Copy link
Copy Markdown
Collaborator

Follow-up to #1341, which fixed implicit Optional hints across VespaSync/VespaAsync plus Vespa.query and Vespa.feed_data_point, but left four methods on the top-level Vespa class declaring namespace/groupname as str while defaulting them to None.

Those four are the most user-facing entry points in the library, and vespa/py.typed ships these signatures to downstream type checkers — so correct, documented usage produced errors in users' editors:

ns: Optional[str] = None
app.get_data(schema="s", data_id="1", namespace=ns)
# error: Argument "namespace" has incompatible type "str | None"; expected "str"

Why these annotations were wrong

None was always the intended value:

  1. The docstrings already say so — e.g. namespace (str, optional): ... If no namespace is provided, the schema is used.
  2. get_document_v1_path explicitly falls back to the schema name when namespace is falsy.
  3. All four methods forwarded the argument verbatim into the VespaSync method that chore: fix optional type hints #1341 had already annotated Optional[str] — the wrappers were stricter than what they delegated to.

Scope

File Change
vespa/application.py 7 params on Vespa.delete_data, delete_all_docs, get_data, update_data
vespa/configuration/vt.py VT.__init__(attrs) — same bug class; body does attrs or {}
pyproject.toml enable Ruff RUF013
tests/integration/ × 2 two helper signatures the new rule flagged

The last three are bundled deliberately: enabling RUF013 is what stops this from drifting back, and the rule fails on vt.py and the two test helpers, so they have to be fixed in the same commit or CI breaks on the next contributor's push. Ruff is already wired into pre-commit, so this adds no dependency and no new CI job.

Uses Optional[...] rather than PEP 604 str | None for consistency with #1341 and the surrounding code (351 vs 10 occurrences in vespa/). A full PEP 604 migration is viable — requires-python is >=3.10 — but it's a ~1,200-line diff across 14 files and belongs in its own change.

Verification

  • pyright (the engine behind Pylance/VS Code) and mypy: both 0 errors on the reproducer above, which produced 4 errors each before.
  • 668 unit tests pass; 51 doctests pass; integration tests collect cleanly.
  • Internal mypy errors in the touched modules drop 72 → 64, with none added.
  • ruff check . clean with RUF013 active; pre-commit hooks pass.

No runtime behaviour changes — annotations are not evaluated for defaults, and every affected parameter already accepted None.

Follow-up to #1341, which fixed VespaSync/VespaAsync plus Vespa.query and
Vespa.feed_data_point but left four methods on the top-level Vespa class
declaring `namespace`/`groupname` as `str` while defaulting them to None.

Those four are the most user-facing entry points in the library, and
vespa/py.typed ships these signatures to downstream type checkers, so
correct and documented usage produced editor errors:

    ns: Optional[str] = None
    app.get_data(schema="s", data_id="1", namespace=ns)  # error before

None was always the intended value: the docstrings already say
"(str, optional) ... If no namespace is provided, the schema is used",
get_document_v1_path falls back to the schema name when namespace is
falsy, and all four forwarded the argument verbatim into the VespaSync
methods that #1341 had already annotated as Optional[str] -- so the
wrappers were stricter than what they delegated to.

Uses Optional[...] rather than PEP 604 `str | None` for consistency with
#1341 and the surrounding code (351 vs 10 occurrences in vespa/).

Also fixes VT.__init__(attrs) in vespa/configuration/vt.py and two
integration-test helpers, and enables Ruff's RUF013 so this cannot drift
back in -- it is already wired into pre-commit, needs no new dependency,
and flagged exactly these sites.

Verified: pyright (the Pylance engine) and mypy both clean on the
reproducer; 668 unit tests pass; internal mypy errors in the touched
modules drop from 72 to 64 with none added.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
thomasht86 and others added 2 commits August 27, 2026 11:20
The Slack workspace URL returns 403 to the docs linkchecker (it blocks
unauthenticated clients), failing mk-docs-test on
Matryoshka_embeddings_in_Vespa-cloud.ipynb. The link itself is fine, so
ignore it rather than change the docs.

Also drops the explanatory comment above the RUF013 entry in
pyproject.toml, per review.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mk-docs-test has been red on master since f30717e (2026-08-21), not
because of broken links but because the docs sweep trips rate limiting:
of 36 reported errors, 34 were huggingface.co returning 429 to the CI
runner, one was http://beir.ai returning 403, and one was our own
llms-full.txt returning 503 (it serves 200 on demand -- 1.9 MB, so
likely a timeout under load).

429 and 503 mean the server answered but declined to serve us right
now; neither says the link is broken. Accepting both keeps genuine 404
detection on huggingface.co links, which adding the host to
ignore_urls.txt would have thrown away -- and pyvespa's docs reference
a lot of HF datasets and models that can genuinely rot.

beir.ai blocks non-browser clients outright, so it goes in the ignore
list alongside the Slack workspace URL.

Verified locally with the same invocation CI uses: 2953 links, 0 errors.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@thomasht86
thomasht86 marked this pull request as ready for review August 27, 2026 10:31
@thomasht86
thomasht86 merged commit d5e173f into master Sep 1, 2026
30 checks passed
@thomasht86
thomasht86 deleted the fix/remaining-implicit-optionals branch September 1, 2026 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants