chore: fix remaining implicit Optional type hints, enforce with RUF013 - #1343
Merged
Merged
Conversation
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>
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
marked this pull request as ready for review
August 27, 2026 10:31
sebastiannberg
approved these changes
Aug 27, 2026
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 #1341, which fixed implicit
Optionalhints acrossVespaSync/VespaAsyncplusVespa.queryandVespa.feed_data_point, but left four methods on the top-levelVespaclass declaringnamespace/groupnameasstrwhile defaulting them toNone.Those four are the most user-facing entry points in the library, and
vespa/py.typedships these signatures to downstream type checkers — so correct, documented usage produced errors in users' editors:Why these annotations were wrong
Nonewas always the intended value:namespace (str, optional): ... If no namespace is provided, the schema is used.get_document_v1_pathexplicitly falls back to the schema name whennamespaceis falsy.VespaSyncmethod that chore: fix optional type hints #1341 had already annotatedOptional[str]— the wrappers were stricter than what they delegated to.Scope
vespa/application.pyVespa.delete_data,delete_all_docs,get_data,update_datavespa/configuration/vt.pyVT.__init__(attrs)— same bug class; body doesattrs or {}pyproject.tomlRUF013tests/integration/× 2The last three are bundled deliberately: enabling
RUF013is what stops this from drifting back, and the rule fails onvt.pyand 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 604str | Nonefor consistency with #1341 and the surrounding code (351 vs 10 occurrences invespa/). A full PEP 604 migration is viable —requires-pythonis>=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) andmypy: both 0 errors on the reproducer above, which produced 4 errors each before.mypyerrors in the touched modules drop 72 → 64, with none added.ruff check .clean withRUF013active; pre-commit hooks pass.No runtime behaviour changes — annotations are not evaluated for defaults, and every affected parameter already accepted
None.