Skip to content

Comments

fix: typing for updated datasets and relations Protocols #2870

Merged
zilto merged 1 commit intodevelfrom
fix/protocol-typing
Jul 14, 2025
Merged

fix: typing for updated datasets and relations Protocols #2870
zilto merged 1 commit intodevelfrom
fix/protocol-typing

Conversation

@zilto
Copy link
Collaborator

@zilto zilto commented Jul 13, 2025

Description

Follow-up to this discussion

  • As mentioned, protocols need to directly inherit from typing.Protocol (source, reported by pylance in the screenshots I shared in the previous discussion).
  • mypy didn't fail because Relation was inadvertently not a Protocol. Similarly, some tests were making assumptions against Relation with isinstance() calls (which wouldn't be possible if it was a proper protocol).
  • DataAccess was implementing method such as __init__ and property setters, which is not possible on a Protocol. To maintain the desired approach, I created a DBApiCursorProtocol that is inherited by the abstract base class DBApiCursor.
  • I removed the uncessary else block after if TYPE_CHECKING
  • import from __future__ import annotations to avoid circular imports

Additional context

I still in favor of removing this abstraction layer as long as we have a single set of Dataset and Relation implementations. Having to modify their superclass / protocols makes maintenance more complex

@netlify
Copy link

netlify bot commented Jul 13, 2025

Deploy Preview for dlt-hub-docs canceled.

Name Link
🔨 Latest commit 423f916
🔍 Latest deploy log https://app.netlify.com/projects/dlt-hub-docs/deploys/687329748174130008cfed9c

@zilto zilto self-assigned this Jul 13, 2025
@zilto zilto requested review from rudolfix and sh-rp and removed request for sh-rp July 13, 2025 03:35
Copy link
Collaborator

@rudolfix rudolfix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

we have Protocols mostly to hide implementation details in concrete classes. several functions should not be exposed to end user or we are still not sure if that is the final form

@rudolfix
Copy link
Collaborator

@sh-rp should look here as well!

@sh-rp
Copy link
Collaborator

sh-rp commented Jul 14, 2025

Thanks @zilto for fixing this! My plan was to allow any object that conforms to "Relation" to be able to be consumed in the transformations, but probably nobody is going to implement their own Relation anyway at this point so for me this is good.

@zilto zilto merged commit 5b6caec into devel Jul 14, 2025
59 checks passed
@zilto zilto deleted the fix/protocol-typing branch July 14, 2025 11:25
zilto added a commit that referenced this pull request Jul 22, 2025
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.

3 participants