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.

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

Super fast snippet linting & type checking #2019

Merged
merged 11 commits into from
Nov 4, 2024
Merged

Conversation

sh-rp
Copy link
Collaborator

@sh-rp sh-rp commented Nov 3, 2024

Description

Lints and typechecks all docs snippets in one go. For this all snippets of each docs page are rendered into one python file and all those files have the same structure as the docs pages. Then the lint and typechecking commands are run on that full folder tree in one command each.

There is one benefit to this that I thought of when writing the dataset docs: For snippets on one docs page that relate to each other (which is a good way of writing pages anyway imho) we can typecheck across them and also make sure that variables stay the same type across one docs page. I think this will need a bit of more work from me though.

ToDo:

  • Adjust some typechecking errors

@sh-rp sh-rp requested a review from burnash November 3, 2024 21:52
@sh-rp sh-rp self-assigned this Nov 3, 2024
Copy link

netlify bot commented Nov 3, 2024

Deploy Preview for dlt-hub-docs canceled.

Name Link
🔨 Latest commit b84e50c
🔍 Latest deploy log https://app.netlify.com/sites/dlt-hub-docs/deploys/67291b24544adb0008b2f412

@sh-rp sh-rp force-pushed the docs/improved_snippet_linting branch from 566b1e7 to 15f232f Compare November 4, 2024 15:46
@sh-rp sh-rp marked this pull request as ready for review November 4, 2024 16:58
@@ -90,7 +90,7 @@ The destination decorator supports settings and secrets variables. If you, for e

```py
@dlt.destination(batch_size=10, loader_file_format="jsonl", name="my_destination")
def my_destination(items: TDataItems, table: TTableSchema, api_key: dlt.secrets.value) -> None:
def my_destination(items: TDataItems, table: TTableSchema, api_key: str = dlt.secrets.value) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice!

@@ -200,7 +202,7 @@ If you wish to create your own pipelines, you can leverage source and resource m
base_id = "Please set me up!" # The ID of the base.

airtables = airtable_source(base_id=base_id)
load_info = pipeline.run(load_data, write_disposition="replace")
load_info = pipeline.run(airtables, write_disposition="replace")
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@@ -236,7 +236,7 @@ To create your data pipeline using single loading and [incremental data loading]
1. To load data from a specific date, including dependent endpoints:

```py
load_data = workable_source(start_date=datetime(2022, 1, 1), load_details=True)
load_data = workable_source(start_date=DateTime(2022, 1, 1), load_details=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here, the type is correct, but now it's not clear where DateTime is coming from. Maybe it'd be better to explicitly use pendulum.DateTime?

burnash
burnash previously approved these changes Nov 4, 2024
Copy link
Collaborator

@burnash burnash left a comment

Choose a reason for hiding this comment

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

Awesome work! I didn't know we have so many bugs in the examples.
Please check my comment regarding pendulum.DateTime–could be a potential readability improvement.

@sh-rp sh-rp force-pushed the docs/improved_snippet_linting branch from 8cc075a to b84e50c Compare November 4, 2024 19:06
@sh-rp sh-rp merged commit b732eb2 into devel Nov 4, 2024
58 of 60 checks passed
@sh-rp sh-rp deleted the docs/improved_snippet_linting branch November 4, 2024 19:19
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.

2 participants