Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor(ingestion): improve Source.create type hints #8620

Closed
wants to merge 1 commit into from
Closed

refactor(ingestion): improve Source.create type hints #8620

wants to merge 1 commit into from

Conversation

LucasRoesler
Copy link
Contributor

Use a Generic variable for the cls and return value of the Source.create method. This allows type hinters, such as Pyright/VSCode, to correctly predict the return value of Source implementations. Previously, it would only return Source, preventing implementations from correctly referencing itself.

Closes #8358

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
  • For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

@github-actions github-actions bot added the ingestion PR or Issue related to the ingestion of metadata label Aug 13, 2023
hsheth2
hsheth2 approved these changes Aug 13, 2023
View reviewed changes
Copy link
Collaborator

@hsheth2 hsheth2 left a comment

Choose a reason for hiding this comment

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

LGTM

Looks like there's a lint issue - it needs double quotes instead of single quotes

@LucasRoesler
refactor(ingestion): improve Source.create type hints
a626a96 
Use a Generic variable for the `cls` and return value of the
`Source.create` method. This allows type hinters, such as
Pyright/VSCode, to correctly predict the return value of Source
implementations. Previously, it would only return `Source`, preventing
implementations from correctly referencing itself.

Closes datahub-project#8358

Signed-off-by: Lucas Roesler <lucas@contiamo.com>
@LucasRoesler LucasRoesler force-pushed the feat-improve-source-create-method-type-hints branch from b9c2af0 to a626a96 Compare August 13, 2023 19:39
@LucasRoesler
Copy link
Contributor Author

LGTM

Looks like there's a lint issue - it needs double quotes instead of single quotes

Sorry, I thought VSCode had already formatted it. I ran black on the file and updated the branch.

@hsheth2
Copy link
Collaborator

hsheth2 commented Aug 14, 2023

Looks like we may need to tighten our type annotations in a couple other places in the codebase:

+ mypy --show-traceback --show-error-codes src/ tests/ examples/
src/datahub/ingestion/source/tableau.py:2542: error: Return type "Source" of "create" incompatible with return type "TableauSource" in supertype "Source"  [override]
src/datahub/ingestion/source/redash.py:376: error: Return type "Source" of "create" incompatible with return type "RedashSource" in supertype "Source"  [override]
src/datahub/ingestion/source/nifi.py:397: error: Return type "Source" of "create" incompatible with return type "NifiSource" in supertype "Source"  [override]
src/datahub/ingestion/source/mode.py:795: error: Return type "Source" of "create" incompatible with return type "ModeSource" in supertype "Source"  [override]
src/datahub/ingestion/source/metabase.py:652: error: Return type "Source" of "create" incompatible with return type "MetabaseSource" in supertype "Source"  [override]
src/datahub/ingestion/source/superset.py:193: error: Return type "Source" of "create" incompatible with return type "SupersetSource" in supertype "Source"  [override]
src/datahub/ingestion/source/kafka_connect.py:1004: error: Return type "Source" of "create" incompatible with return type "KafkaConnectSource" in supertype "Source"  [override]
src/datahub/ingestion/source/delta_lake/source.py:125: error: Return type "Source" of "create" incompatible with return type "DeltaLakeSource" in supertype "Source"  [override]
tests/unit/test_pipeline.py:393: error: Return type "Source" of "create" incompatible with return type "FakeSource" in supertype "Source"  [override]
src/datahub/ingestion/source/snowflake/snowflake_v2.py:297: error: Return type "Source" of "create" incompatible with return type "SnowflakeV2Source" in supertype "Source"  [override]

@maggiehays maggiehays added community-contribution PR or Issue raised by member(s) of DataHub Community pending-submitter-response Issue/request has been reviewed but requires a response from the submitter labels Aug 15, 2023
@shirshanka shirshanka added the on-deck PR or Issue that will be reviewed and/or addressed by the DataHub Maintainers in future cycles label Jun 28, 2024
@datahub-cyborg datahub-cyborg bot added merge-pending-ci A PR that has passed review and should be merged once CI is green. and removed pending-submitter-response Issue/request has been reviewed but requires a response from the submitter labels Dec 24, 2024
hsheth2 added a commit that referenced this pull request Jan 11, 2025
@hsheth2
fix(ingest): tighten Source.create type annotations
bca63db 
Supersedes #8620
@hsheth2 hsheth2 mentioned this pull request Jan 11, 2025
Open
5 tasks
@hsheth2
Copy link
Collaborator

hsheth2 commented Jan 11, 2025

Closing in favor of #12325

@hsheth2 hsheth2 closed this Jan 11, 2025
@codecov Codecov
Copy link

codecov bot commented Jan 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Files with missing lines Coverage Δ
...data-ingestion/src/datahub/ingestion/api/source.py 98.56% <100.00%> (+0.01%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d06ca6...a626a96. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hsheth2 hsheth2 hsheth2 approved these changes

Assignees

@hsheth2 hsheth2

Labels
community-contribution PR or Issue raised by member(s) of DataHub Community ingestion PR or Issue related to the ingestion of metadata merge-pending-ci A PR that has passed review and should be merged once CI is green. on-deck PR or Issue that will be reviewed and/or addressed by the DataHub Maintainers in future cycles
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Type hinting on Source.create doesn't respect inheritance
4 participants
@LucasRoesler @hsheth2 @shirshanka @maggiehays