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

Add Toast example #70

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

fivetran-clgritton
Copy link
Collaborator

@fivetran-clgritton fivetran-clgritton commented Jan 8, 2025

Many customers are going to be asking for this for the Toast API. This is a very simple example and is not a replacement for the existing connector.

image

Copy link

height bot commented Jan 8, 2025

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

examples/source_examples/toast/configuration.json Outdated Show resolved Hide resolved
examples/source_examples/toast/connector.py Show resolved Hide resolved
Comment on lines 48 to 50
to_ts = datetime.datetime.now(datetime.timezone.utc).isoformat("T", "milliseconds")
if 'to_ts' in state:
from_ts = state['to_ts']
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can rename these variables for better code readability

Copy link
Collaborator

Choose a reason for hiding this comment

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

Names updated for both to_ts and from_ts.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just from reading these variables, it is not clear. Can we rename it to start_time and end_time or something similar?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Updated to start_utc_timestamp and current_utc_timestamp.

Copy link
Collaborator

@fivetran-satvikpatil fivetran-satvikpatil left a comment

Choose a reason for hiding this comment

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

Please add testing details in PR description.

@@ -0,0 +1,202 @@

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a line mentioning what this example does.

def update(configuration: dict, state: dict):

try:
conf = configuration
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason for introducing another variable?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No reason - I've updated the code to just use the parameter name itself.

Comment on lines 48 to 50
to_ts = datetime.datetime.now(datetime.timezone.utc).isoformat("T", "milliseconds")
if 'to_ts' in state:
from_ts = state['to_ts']
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just from reading these variables, it is not clear. Can we rename it to start_time and end_time or something similar?

@fivetran-satvikpatil
Copy link
Collaborator

Also, please update the README file with the example details.

Copy link
Collaborator

@varundhall varundhall left a comment

Choose a reason for hiding this comment

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

Almost there

Comment on lines 1 to 2
fivetran_connector_sdk==0.13.10.1
Requests==2.32.3
Copy link
Collaborator

Choose a reason for hiding this comment

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

if these are the only 2 requirements then we can skip this file

Comment on lines 2 to 6
"clientId": "",
"clientSecret": "",
"userAccessType": "",
"domain": ""
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

please add placeholder text like our other examples

# - state: a dictionary contains whatever state you have chosen to checkpoint during the prior sync
# The state dictionary is empty for the first sync or for any full re-sync
def update(configuration: dict, state: dict):

Copy link
Collaborator

Choose a reason for hiding this comment

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

please add the examples log, refer other examples

@orizwanft orizwanft requested a review from varundhall February 5, 2025 16:24
Copy link
Collaborator

@varundhall varundhall left a comment

Choose a reason for hiding this comment

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

Almost there, please address these minor comments

Comment on lines +4 to +6
# This is an example to extract data from Toast, technology platform primarily designed for the restaurant industry.
# It provides an all-in-one point-of-sale (POS) and management system tailored to meet the unique needs
# of restaurants, cafes, and similar businesses.
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should go above the line 2 and 3, see other examples for reference

# from the correct position in case of interruptions.
yield op.checkpoint(state)

# The function takes six parameters:
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should be consistent in comments, please explain what this method does, can be a one-liner

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.

4 participants