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

TF-1627 Update same_query_across_sites example #35

Merged
merged 7 commits into from
Aug 13, 2024

Conversation

frankfeng98
Copy link
Contributor

@frankfeng98 frankfeng98 commented Aug 8, 2024

This fixes TF-1627

This PR fixed same_query_across_sites.py example, which shows how to compare price across websites with AgentQL synchronously (we have another async example). Nothing changed significantly in this PR other than making it use the new Smart Locator API.

@rachelnabors rachelnabors requested a review from colriot August 9, 2024 15:08
@frankfeng98 frankfeng98 changed the title Update same_query_across_sites example TF-1627 Update same_query_across_sites example Aug 9, 2024
Copy link
Contributor

@colriot colriot left a comment

Choose a reason for hiding this comment

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

Let's update the output printing code, pls


def print_header():
"""Prints the header for the data table"""
print(f"{'Website':<25} | {'Product ':<20} | {'Price ':<20} ")
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use https://pypi.org/project/prettytable/ to not reinvent the wheel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a little hesitant to use prettytable library here because it's not included in Python standard library and requires pip install prettytable. I guess it depends on what kind of customer journey we want to provide to users. If we expect users to find this example through link on our documentation website and then copy / paste the code into their project. Then any extra setup requirement would create an issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's a matter of providing clear instructions. In reality people would not use our code to print tables, they probably won't print tables to console at all, tbh. You can rewrite this code in a shape of preparing the data first (which everyone can copy-paste, and then outputting them second (which can be ignored).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood! You're absolutely right that people would not print tables. In this case, I will probably remove the outputting table part entirely. Because I feel like even with clear instructions, there will be a fairly large amount of users who will just copy/paste the code, run it, find out the code does not work, and then try to figure out what's going on. And the purpose of these examples is just to demonstrate the core functionality of AgentQL.

browser = playwright.chromium.launch(headless=False)

# Create a new AgentQL page instance in the browser for web interactions
page: Page = browser.new_page()
Copy link
Contributor

Choose a reason for hiding this comment

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

fix the linter, pls

{
price
}
}"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
}"""
}
"""

print_header()

with sync_playwright() as playwright:
browser = playwright.chromium.launch(headless=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

not specific to this example, but what do you think about moving browser creation into with block as well, to automanage it as a resource?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's a good idea and will save users the pain of both creating browser and stopping browser.

But to do this, we probably need to create our own context manager API that wraps around the launch() method to make it work in both sync and async environment.

Copy link
Contributor

Choose a reason for hiding this comment

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

It works out of the box

@frankfeng98 frankfeng98 requested a review from colriot August 12, 2024 20:08
python3 compare_product_prices.py
```

## Play with it
Copy link
Contributor

Choose a reason for hiding this comment

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

This heading can be confusing. First heading is "Run the script", second "Play with it" reads like "Play with the script". What about "Play with the query"?

Copy link
Contributor

Choose a reason for hiding this comment

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

That is a great suggestion!

# Use query_data() method to fetch the price from the BestBuy page
response = page.query_data(PRODUCT_INFO_QUERY)

print_row("BestBuy", "Nintendo Switch", response["nintendo_switch"]["price"])
Copy link
Contributor

Choose a reason for hiding this comment

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

We should fix the source of these check failures

@frankfeng98 frankfeng98 requested a review from colriot August 12, 2024 23:51
Copy link
Contributor

@rachelnabors rachelnabors left a comment

Choose a reason for hiding this comment

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

Thanks for working together on this. Loving it

@frankfeng98 frankfeng98 dismissed colriot’s stale review August 13, 2024 20:25

All comments addressed and unnecessary table output has been removed

@frankfeng98 frankfeng98 merged commit 923271e into main Aug 13, 2024
1 check passed
@frankfeng98 frankfeng98 deleted the update_same_query_across_sites branch August 13, 2024 20:25
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