Add structural Protocol types for TSC item classes - #1802
Conversation
…gs type - Rename DefaultPermissionsEndpoint's local BaseItem alias to DefaultPermissionsTarget to avoid shadowing the new public Protocol - Remove _initial_tags from TaggableItem (internal dirty-tracking detail, not a public contract); update ContentItem docstring to include MetricItem - Narrow WorkbookItem._initial_tags and DatasourceItem._initial_tags annotations from bare set to set[str] for Protocol invariance compliance Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… __all__ Taggable in resource_tagger.py was never used as a type bound -- TaggableItem in base_item.py now covers the public contract. Also remove runtime_checkable import which became unused. Fix pre-existing duplicate SiteOIDCConfiguration entry in models/__init__.__all__. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ViewItem.owner_id is not independently writable (it tracks the parent workbook's owner), so a plain writable Protocol attribute annotation would mislead mypy. A @Property annotation satisfies both ViewItem's read-only property and the writable instance attributes on other item classes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add test/test_protocols.py covering isinstance() checks for BaseItem, OwnedItem, TaggableItem, and ContentItem against all representative item classes (WorkbookItem, DatasourceItem, ViewItem, FlowItem, ProjectItem, MetricItem, UserItem) and plain structural objects, including negative cases and protocol-hierarchy checks. - Add _TaggableWithInitial private Protocol in resource_tagger.py to capture the _initial_tags implementation detail alongside the public TaggableItem surface; use it to fully annotate _ResourceTagger._add_tags, _delete_tag, and update_tags. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…test Add section comments to __init__.__all__ to distinguish the new structural protocols from the alphabetical concrete-model list. Move the inline `import datetime` in test_protocols.py to module level, matching the import style used throughout the test suite. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
This is great and will make other typing easier. Should they be named differently to clearly differentiate them? Something like |
|
Hm, I think underscores would make some more helpful IDEs hide/limit them in autocomplete etc, so not that. A common approach is something like "SupportsOwner", but that doesn't quite work for "ContentItem". I think the current item works ok and is pretty standard. |
…es.py
BaseItem is renamed to TableauItem to match the existing name in
tableau_types.py and fulfill its TODO comment ("should define TableauItem
as an interface"). The Union type alias is replaced with an import of the
Protocol, removing the need to enumerate concrete types.
id and name are now declared as @Property on the Protocol so that concrete
classes with read-only property implementations (and VirtualConnectionItem
whose name: str is narrower than str | None) satisfy it under mypy's
covariant property checking.
A private _PermissibleItem Protocol is added to permissions_endpoint.py
to type the populate() method's _set_permissions call, which is an
implementation detail not appropriate for the public TableauItem Protocol.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- base_item.py TableauItem docstring: fix the stale first paragraph that claimed "plain Protocol attributes (not @Property)". The code uses @Property; the docstring now matches. Also documents that TaskItem and DataAlertItem (no `name` attribute) don't satisfy the protocol, and calls out the issubclass-raises-TypeError gotcha for data-attribute Protocols. - __init__.py: export ContentItem, OwnedItem, TaggableItem from the top-level package. Previously only TableauItem was accessible as TSC.TableauItem; the other three were only under models/__init__.py, which meant TSC.ContentItem raised AttributeError. - test_protocols.py: add coverage for previously-untested types the original Union included. DatabaseItem, TableItem, VirtualConnectionItem positive on TableauItem; VirtualConnectionItem positive on OwnedItem and negative on TaggableItem (no `tags` attribute). TaskItem negative on TableauItem (has `id` but no `name`), validating the docstring's explicit exclusion. Plus an issubclass-raises-TypeError test to lock in the data-attribute Protocol limitation. 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.
Adds structural typing.Protocol interfaces for Tableau resource items and updates internal endpoints/tests to rely on these protocols for runtime and static type checks.
Changes:
- Introduces
TableauItem,OwnedItem,TaggableItem, andContentItemstructural protocols inmodels/base_item.pyand exports them via package inits. - Refactors endpoint typing to use private structural protocols (permissions/tagging) and improves tag typing in model items.
- Adds a new pytest suite validating runtime
isinstance()behavior for the new protocols.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_protocols.py | Adds tests validating runtime structural protocol conformance across representative items. |
| tableauserverclient/server/endpoint/resource_tagger.py | Replaces the old runtime-checkable protocol with a private protocol and tightens method type hints. |
| tableauserverclient/server/endpoint/permissions_endpoint.py | Introduces a private protocol for items that support permission population and updates populate() typing. |
| tableauserverclient/server/endpoint/default_permissions_endpoint.py | Renames the BaseItem alias to a clearer DefaultPermissionsTarget. |
| tableauserverclient/models/workbook_item.py | Tightens _initial_tags typing to set[str]. |
| tableauserverclient/models/datasource_item.py | Tightens _initial_tags typing to set[str]. |
| tableauserverclient/models/tableau_types.py | Removes the old Union[...] TableauItem alias and documents the new Protocol-based approach. |
| tableauserverclient/models/base_item.py | Adds the new structural Protocol definitions. |
| tableauserverclient/models/init.py | Re-exports the new protocols and updates imports/__all__. |
| tableauserverclient/init.py | Exposes the new protocols at the top-level package API. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from tableauserverclient.server.server import Server | ||
|
|
||
|
|
||
| class _TaggableWithInitial(Protocol): |
| id: str | None | ||
| tags: set[str] | ||
| _initial_tags: set[str] |
| "ServerInfoItem", | ||
| "SiteAuthConfiguration", | ||
| "SiteItem", | ||
| "SiteOIDCConfiguration", | ||
| "SubscriptionItem", | ||
| "TableItem", | ||
| "TableauAuth", | ||
| "PersonalAccessTokenAuth", | ||
| "JWTAuth", | ||
| "Resource", | ||
| "TableauItem", | ||
| "plural_type", |
| class Tagged: | ||
| id: str | None = None | ||
| name: str | None = "x" | ||
| tags: set = set() |
Adds a release-note entry for the isinstance() semantics change so users who relied on the TypeError from isinstance(x, Union) will see it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Motivation
tableau_types.pyhas carried a# todo: refactoring: should actually define TableauItem as an interface...comment for years. The currentTableauItemis aUnion[DatasourceItem, FlowItem, ...]which forcesevery new item type to be enumerated by hand and doesn't compose well
with tag/permission helpers that only need a subset of attributes.
Extracting a Protocol hierarchy (
TableauItem,OwnedItem,TaggableItem,ContentItem) closes that TODO and gives the internalhelpers (
resource_tagger,permissions_endpoint.populate) a realtype they can name without importing every concrete class.
Behavior change
Public
TableauItemsemantics change. It was a Union, now it's astructural Protocol. Two observable effects for downstream code:
now accept any object with
idandname-- widening.isinstance(x, TableauItem)previously raisedTypeError(Unionisn't a runtime-checkable class); it now returns
Truefor anyobject with the right shape. If you relied on the
TypeErroras aguard, add an explicit concrete-type check instead.
Called out in
CHANGELOG.mdin this PR.Also bundled (cosmetic):
resource_tagger's privateTaggableprotocol replaced by_TaggableWithInitialdefault_permissions_endpoint's localBaseItemtype alias renamedto
DefaultPermissionsTarget_initial_tags: setnarrowed toset[str]inDatasourceItem/WorkbookItemSiteOIDCConfigurationentry removed frommodels.__all__(pre-existing bug)Test plan
test/test_protocols.pycovers positive cases (workbook, datasource,view, flow, metric satisfy the Protocols), negative cases (TaskItem /
DataAlertItem don't satisfy TableauItem because they lack
name),and the
issubclass -> TypeErrorgotchaTSC.TableauItem,TSC.ContentItem,TSC.OwnedItem,TSC.TaggableItemall reachable from top-level packagets-api_3_29.xsdprimary content types(workbook/datasource/view/flow/metric) all carry id, name, owner, tags,
createdAt/updatedAt -- Protocol hierarchy matches
🤖 Generated with Claude Code