feat: consolidate and export public type aliases (HyperAction, FilePath, AddResponse, IDP types) - #1825
feat: consolidate and export public type aliases (HyperAction, FilePath, AddResponse, IDP types)#1825jacalata wants to merge 3 commits into
Conversation
|
Claude Code posted this on my behalf after review: OverviewThe PR title/description says this consolidates five families of previously-duplicated type aliases (
This looks like squashed/rebased history from other branches (commit messages like "Create Sphinx configuration file for documentation," "add more e2e tests," "change theme" appear in What's good about the core type-consolidation change
Issues found
SecurityNothing security-sensitive in the type-consolidation change itself. The new RecommendationThe type-consolidation refactor itself is sound (verified by tests/mypy/black) and worth merging, but I'd ask the author to confirm the diff scope before approval — specifically, whether this branch should be rebased to drop the docs/e2e/docstring commits so this PR reviews cleanly as "consolidate and export public type aliases," matching its title and description. |
|
@jacalata see the things claude code pointed out above -- in particular are we sure the "docs" changes should be in here? (Docs are still in gh-pages branch.) |
…ient.types HyperAction*, FilePath, FileObject*, PathOrFile*, AddResponse, IDPAttributes, IDPProperty, and HasIdpConfigurationID were defined locally in endpoint files (in some cases duplicated across 4 files) but not importable from the top-level package. Users annotating calls to update_hyper_data(), publish(), download(), add_to_schedule(), or OIDC methods had to import from internal modules. Move all shared type aliases to tableauserverclient/types.py and re-export from tableauserverclient/__init__.py. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
73d225c to
e418c61
Compare
|
cleaned up, docs work now lives in #1832. |
…ants Two adversarial-review cleanups on tableauserverclient/types.py: - HasIdpConfigurationID was `str | IDPAttributes`, and every caller annotation used `str | HasIdpConfigurationID`, which expanded to the redundant `str | str | IDPAttributes`. Narrowed the alias to a pure Protocol union so callers can write `str | HasIdpConfigurationID` without stuttering. - IDPAttributes covers objects that declare `idp_configuration_id` as a plain class attribute; IDPProperty covers objects that declare it as a read-only @Property. Both were introduced in #1630 but only IDPAttributes was ever in HasIdpConfigurationID's union, so the @Property variant would fail mypy against the endpoint's public signature. HasIdpConfigurationID now unions both to match the original design intent. - Added a docstring block explaining why both Protocol variants exist. - Cleaned up unused IDPAttributes/IDPProperty imports in oidc_endpoint.py (only HasIdpConfigurationID is referenced there). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR centralizes commonly repeated typing constructs (file/path unions, Hyper action payloads, schedule AddResponse, and OIDC IDP protocols) into a new tableauserverclient.types module and updates endpoints to import from it.
Changes:
- Added
tableauserverclient/types.pycontaining shared type aliases, TypedDicts, and Protocols. - Replaced duplicated type definitions across multiple endpoints with imports from
tableauserverclient.types. - Re-exported the new shared types from
tableauserverclient/__init__.pyfor public consumption.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tableauserverclient/types.py | Introduces shared typing definitions used across endpoints. |
| tableauserverclient/server/endpoint/workbooks_endpoint.py | Switches local file/path and AddResponse typing to centralized imports. |
| tableauserverclient/server/endpoint/schedules_endpoint.py | Moves AddResponse definition to shared types module. |
| tableauserverclient/server/endpoint/oidc_endpoint.py | Replaces inline OIDC IDP Protocol definitions with shared import. |
| tableauserverclient/server/endpoint/flows_endpoint.py | Switches local file/path and AddResponse typing to centralized imports. |
| tableauserverclient/server/endpoint/datasources_endpoint.py | Switches local file/path and Hyper action typing to centralized imports. |
| tableauserverclient/server/endpoint/custom_views_endpoint.py | Switches local file/path typing to centralized imports. |
| tableauserverclient/init.py | Re-exports shared types as part of the public package API. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # HasIdpConfigurationID unions both so downstream callers can implement either | ||
| # style without hitting mypy invariance errors. Callers accept | ||
| # `str | HasIdpConfigurationID` -- passing a raw id string or an object. | ||
| class IDPAttributes(Protocol): | ||
| idp_configuration_id: str | ||
|
|
||
|
|
||
| class IDPProperty(Protocol): | ||
| @property | ||
| def idp_configuration_id(self) -> str: ... | ||
|
|
||
|
|
||
| HasIdpConfigurationID = IDPAttributes | IDPProperty |
There was a problem hiding this comment.
The alias definition changed, but the call-site type signature was also updated to compensate. Old shape:
HasIdpConfigurationID = str | IDPAttributes
# and at the call site
def get_by_id(self, id: HasIdpConfigurationID) -> ...New shape:
HasIdpConfigurationID = IDPAttributes | IDPProperty
# and at the call site
def get_by_id(self, id: str | HasIdpConfigurationID) -> ...Effective type at the call site went from str | IDPAttributes to str | IDPAttributes | IDPProperty — strictly wider, not narrower. str is now unioned at the parameter rather than inside the alias, which is deliberate: the alias should describe "an object that carries an IDP configuration id" and the str shortcut is separate. Ends up accepting everything the old signature accepted plus the @property-style object shape that mypy invariance previously rejected.
| from typing import Literal, Protocol, TypedDict | ||
|
|
||
| # File path and object type aliases used across publish/download methods | ||
| FilePath = str | os.PathLike |
| from tableauserverclient.types import ( | ||
| AddResponse, | ||
| FilePath, | ||
| FileObject, | ||
| FileObjectR, | ||
| FileObjectW, | ||
| HasIdpConfigurationID, | ||
| HyperAction, | ||
| HyperActionCondition, | ||
| HyperActionRow, | ||
| HyperActionTable, | ||
| IDPAttributes, | ||
| IDPProperty, | ||
| PathOrFile, | ||
| PathOrFileR, | ||
| PathOrFileW, | ||
| ) |
Summary
HyperAction,HyperActionTable,HyperActionRow,HyperActionCondition— needed to annotate theactionsargument todatasources.update_hyper_data()FilePath,FileObject,FileObjectR,FileObjectW,PathOrFile,PathOrFileR,PathOrFileW— needed to annotatepublish()/download()arguments across datasources, workbooks, flows, and custom views; were duplicated across 4 endpoint filesAddResponse— return type ofschedules.add_to_schedule(), also used by the same method on datasources, workbooks, and flowsIDPAttributes,IDPProperty,HasIdpConfigurationID— needed to annotate arguments tooidc.get_by_id()andoidc.delete_configuration()All types are now defined in
tableauserverclient/types.pyand exported fromtableauserverclient/__init__.py. The endpoint files import fromtypes.pyinstead of defining them locally.Test plan
pytest test/)from tableauserverclient import HyperAction, FilePath, AddResponse, HasIdpConfigurationID🤖 Generated with Claude Code